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

Update Library Versions #333

Merged
merged 4 commits into from
Sep 13, 2018
Merged

Update Library Versions #333

merged 4 commits into from
Sep 13, 2018

Conversation

schbetsy
Copy link
Collaborator

@schbetsy schbetsy commented Sep 11, 2018

Ensure that the set of allowed versions contains the actual version that cfgov-refresh is currently on, so when we import and install this repo in cfgov, it doesn't cause version conflicts.

For most libraries, allow any revision within a major version. There are a few exceptions, especially for libraries that drop compatibility with django 1.8 without a major version bump.

Testing

tox -r will run the tests on Django 1.8 and 1.11 with upgraded versions of all affected libraries.

Checklist

  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

@coveralls
Copy link

coveralls commented Sep 11, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling f340146 on library-versions into 27906cc on master.

setup.py Outdated
]


docs_extras = [
'Markdown==2.3.1',
'Markdown==2.6.9',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the docs requirements, I've only upgraded them to match cfgov-refresh instead of doing the whole process of researching each library to find its versioning scheme so I can decide what version range to list. I don't feel very confident making more changes to docs requirements, since they're not covered by tests.

Another option would be to leave them alone entirely (and undo the changes I've made here), since docs requirements aren't pulled in to cfgov-refresh, so version mismatches won't cause conflicts. Do you have opinions on what I should do here, @chosak ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly, although I notice two things that are strange:

  1. Running pip install -e '.[docs,testing]', I see these warnings:

    requests 2.19.1 has requirement certifi>=2017.4.17, but you'll have certifi 2016.8.2 which is incompatible.
    mkdocs 0.16.3 has requirement livereload>=2.5.1, but you'll have livereload 2.3.2 which is incompatible.

  2. The PyYAML versions in install_requires and docs_extras seem to conflict, although they also do in cfgov-refresh.

If you look in the setup.py requirements for mkdocs, the versions there are also different from the ones here in docs_extras. I wonder where the list of docs requirements came from here, maybe someone did a pip freeze on their virtualenv and just stuck those versions in? If I try with this much-reduced list of requirements, keeping only mkdocs and the mkDOCter template:

docs_extras = [
    'mkDOCter==1.0.5',
    'mkdocs==0.16.3',
]

then commands like mkdocs build and mkdocs serve still work to build and serve all of the documentation, seemingly correctly. So maybe we can just reduce these down.

Then again, I agree with you that it's not great that (a) these aren't covered by tests and (b) isn't mentioned in the README, neither how to build locally or how to push changes to the GH-hosted version. So, while we could just leave as-is, it'd be nice to get this working/documented, while we're in here, if it's not too much work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excellent idea. Done in a0b4556 and 3bf0504.

For most libraries, allow any revision within a major version. Ensure
that the set of allowed versions contains the actual version that
cfgov-refresh is currently on, so when we import and install this repo
in cfgov, it doesn't cause version conflicts.

### Notes

- The `mkdocs gh-deploy` command will push any edits you've made locally to the `gh-pages` branch of the repository, whether you've committed them or not.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the thorough documentation on this!

setup.py Outdated
'Unipath==1.1',
'elasticsearch>=2.4.1,<3',
'PyYAML>=3.11,<3.14',
'python-dateutil>=2.2,<3',
Copy link
Member

Choose a reason for hiding this comment

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

Super minor and annoying thing: in cfgov-refresh this is currently ==2.1. The changelog entry for 2.2 is here. I don't really know enough about that library at first glance to know if those are big things we need to worry about or not. But:

It looks to me like the only use of dateutil in this whole repository is in this unit test file. So perhaps this could/should be moved out to testing_extras? If you look into that test file, it looks to me like that library could even be removed in favor of just creating the timestamps directly. So maybe we could even just strip it out?

Copy link
Collaborator Author

@schbetsy schbetsy Sep 13, 2018

Choose a reason for hiding this comment

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

Well spotted. Does this work: f340146? (tox passes, so I hope so...)

Copy link
Member

Choose a reason for hiding this comment

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

I see, so the tests don't even use that timestamp at all. Nice fix.

@schbetsy schbetsy merged commit 09789d9 into master Sep 13, 2018
@schbetsy schbetsy deleted the library-versions branch September 13, 2018 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants