-
Notifications
You must be signed in to change notification settings - Fork 75
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
Fix doc test & build dependency errors #1141
Conversation
3a026fb
to
734f5c4
Compare
734f5c4
to
b94d5d7
Compare
b94d5d7
to
7e4538a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like us to understand the reason why the tests passed in #1137 and failed afterwards on master
. What differs? Until we understand this, all fixes are bound to be uncertain. We cannot iterate on production versions by publishing a new patch every day.
setup.py
Outdated
] | ||
|
||
dev_requirements = [ | ||
'autopep8 >= 1.4.0, < 1.6.0', | ||
'coverage == 6.0.2', | ||
'darglint == 1.8.0', | ||
'flake8 >= 4.0.0, < 4.1.0', | ||
'flake8 >= 3.9.0, < 4.0.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgrading flake8 introduced a dependency conflict with sphinx, that was being silenced by test-doc
.
As described in #1140 (comment), I believe there is no way we currently can reproduce the documented failure, so we cannot know if publishing this version will fix the problem. I was trustful for #1137, but I cannot approve this PR under these circumstances now, especially considering that it currently reverts #1137 yet introduces other untestable dependency changes. I would fast approve a pure #1137 revert, considering that the issue #1136 it fixed was extremely minor (misleading log with no consequence), or could consider approving this if #1140 included a repeatable procedure for testing changes. I would also ask that the |
Yes, it's a revert / and could also be handled as such TBH. I gave some hours to try to understand the issue, and gave up by reverting that specific lib upgrade after taking a closer look at the errors. IMHO the ideal way of being able to catch this sooner is to include doc dependencies in in openfisca-core (for example .[doc]). It's a trade-off, certainly. |
Actually the |
This reverts commit 7e4538a.
Fantastic, so what I get now:
So this is a layer 8 error: instead of downgrading pycodestyle, I proposed upgrading flask8. It concurred with the silencing and we got no clue of what was happing. Let's remove the silencers! |
Now by suppressing the silencers we have actionable logs:
https://github.com/openfisca/openfisca-core/runs/7611320470?check_suite_focus=true Which confirms the need to rollback that upgrade. Rollback or no,
I think this PR now provides an increase in our understanding of what's going on. |
Versions >= 4.0.0 of flake8 require a library version, for clients using Python < 3,8, that is incompatible with the same library requirement by Sphinx >= 4.5.
As I put it in the last commit: Versions >= 4.0.0 of flake8 require a library version, for clients using |
Fixes #1136
Fixes #1140
Technical changes
pycodestyle
dependency in fresh editable installations.flake8
dependency in fresh editable installations.Note: this PR is the continuation of #1137, but seems the solution was not complete. This PR should unblock doc tests failing because of dependency mismatch.