search

replicahq / doppelganger

A Python package of tools to support population synthesizers
Apache License 2.0
165 stars 32 forks source link

Add support for versioning to config files, will raise assertion error if 'version' is missing or not up-to-date in the config_json file. #12

Closed Mogeng closed 7 years ago

Mogeng commented 7 years ago
  1. changed config.py to add the current_version and version check.
  2. changed the sample json file to be aligned with the version check.
  3. Added two test for version check in config.

This change is Reviewable

katbusch commented 7 years ago

Looks great! Thanks for adding this. A couple comments below.

And a note on the title: in general we want the title of the pull request to be as descriptive as possible because it will become the commit message in the commit logs. We want others looking back at the commit history to understand which changes are where. So a little bit more info in the title could help, eg "Add support for versioning to config files"

PS I'm not sure why coveralls is showing your coverage as going down. That seems wrong. I wouldn't pay much attention to it :)

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

doppelganger/config.py, line 52 at r1 (raw file):


        assert 'version' in config_json, 'config_json has no key named version'
        assert config_json['version'] == CURRENT_VERSION, 'config_json version not current'

Since the user will be seeing these error messages and won't have access to the code, let's try to make them as understandable to a user who hasn't read the code. Here are suggestions: "The config file is missing a 'version' field" and "The config file version is incorrect. Please upgrade your config file to version {}.".format(CURRENT_VERSION)

test/test_config.py, line 50 at r1 (raw file):

        config_json = self._mock_config_files()
        config_json['version'] = '1'
        self.assertRaises(AssertionError, Configuration.from_json, config_json)

Awesome, great tests!

Can you add a test where the creation of a config succeeds?

Comments from Reviewable

Mogeng commented 7 years ago

Changed the commit title to "Updated support for versioning to config files, error message more clear and added a success test case."

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

doppelganger/config.py, line 52 at r1 (raw file):

Previously, katbusch (Kat Busch) wrote…
Since the user will be seeing these error messages and won't have access to the code, let's try to make them as understandable to a user who hasn't read the code. Here are suggestions: `"The config file is missing a 'version' field"` and `"The config file version is incorrect. Please upgrade your config file to version {}.".format(CURRENT_VERSION)`

Changed the assertion error message more clear and understandable.

test/test_config.py, line 50 at r1 (raw file):

Previously, katbusch (Kat Busch) wrote…
Awesome, great tests! Can you add a test where the creation of a config succeeds?

Added a test case where the correct config_json works.

Comments from Reviewable

katbusch commented 7 years ago

Looks good! Did you see my note about change the title of the pull request?

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Mogeng commented 7 years ago

Oops, just updated the title of the pull request. Thought it referred to the title of the commit...

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

katbusch commented 7 years ago
:lgtm:

Great!

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable