Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert to temple #50

Merged
merged 15 commits into from
Aug 23, 2018
Merged

Convert to temple #50

merged 15 commits into from
Aug 23, 2018

Conversation

multigl
Copy link
Contributor

@multigl multigl commented Aug 20, 2018

  • Temple
  • Circle CI 2.0

@@ -0,0 +1,9 @@
_extensions: [jinja2_time.TimeExtension]
_template: git@github.com:CloverHealth/temple-python-public.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@multigl
Copy link
Contributor Author

multigl commented Aug 21, 2018

resolves #5 #7 #16

@jiffyclub
Copy link

You'll need to add installation of Postgres to the test setup (it's not part of the template), you can see an example here: https://github.com/CloverHealth/pgmock/blob/5c0d2be637c23729a306c0346b9ec1340450a54b/.circleci/config.yml#L17-L22.

@multigl multigl closed this Aug 21, 2018
@multigl multigl deleted the convert-to-temple branch August 21, 2018 21:17
@multigl multigl restored the convert-to-temple branch August 21, 2018 21:17
@multigl multigl deleted the convert-to-temple branch August 21, 2018 21:17
@multigl multigl restored the convert-to-temple branch August 21, 2018 21:19
@multigl multigl reopened this Aug 21, 2018
checks_and_deploy:
jobs:
- temple_check
- lint
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- test_py34 is missing here, which causes deploy's requirements to fail below.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if this isn't Python 3.4 compatible, delete that job.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems to have done it

@multigl multigl requested review from bijanvakili and removed request for gwax and dave-clover August 22, 2018 18:21
@multigl multigl requested a review from 6C1 August 22, 2018 18:21
@@ -316,17 +317,16 @@ def test_disallow_flushes_within_clock_ticks_when_strict(self, session, session_
t.prop_a = 2

with pytest.raises(AssertionError) as excinfo:
eval('session.{func_name}()'.format(func_name=session_func_name))
eval('session.{func_name}()'.format(func_name=session_func_name)) # pylint: disable=eval-used
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using eval and string interpolation, could we use getattr(session, session_fun_name)()? Here and throughout.

@@ -0,0 +1,2 @@
CHANGES
=======
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you going to add content here from the past 10 releases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I GUESS

pyenv install -s 3.4.4

# Set up the dev env and the environments for Tox
pyenv local ${MODULE_NAME} 3.6.2 3.5.3 3.4.4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't pyenv local restricted to a single local version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question, this makefile comes from a temple public project, I think it's necessary for tox

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hmm I missed this in my update to the template, testing on CircleCI doesn't use tox anymore (and nor would it locally unless we set that up again).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I'd put that effort in, tox was really challenging to get set up.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, probably worth updating this in the template to only install a recent Python version.

@@ -63,13 +66,15 @@ def __init__(

@property
def clock_table(self):
""" DEPRECATED: use .clock_model instead -- Clock Model for this Model"""
Copy link
Contributor

@bijanvakili bijanvakili Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion (for later if necessary): Consider using decorators or direct warnings for deprecated functionality:
https://docs.python.org/3/library/warnings.html#warnings.warn
https://stackoverflow.com/questions/2536307/decorators-in-the-python-standard-lib-deprecated-specifically

I prefer these because pylint picks them up and shows me them immediately in my IDE so I know to avoid using them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pycharm picks up the warnings.warn on the next line about deprecation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops... I missed that line. Thanks!


assert re.match(
r'.*flush\(\) has triggered for a changed temporalized property outside of a clock tick.*',
str(excinfo)
str(excinfo),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I usually use str(excinfo.value) to get only the exception class rather than the pytest ExceptionInfo wrapper which sometimes doesn't include the specific message.

pytest-mock==1.10.0
pytest==3.7.2
pytest-randomly>=1.0,<2
testing.postgresql>=1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAAAAAS... so much prefer the disposable database approach of testing.postgres 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me too

Copy link
Contributor

@bijanvakili bijanvakili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@multigl multigl merged commit d295d22 into master Aug 23, 2018
@multigl multigl deleted the convert-to-temple branch August 23, 2018 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants