-
Notifications
You must be signed in to change notification settings - Fork 15
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
Restore werkzeug dependency (fixes #38) #39
Conversation
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.
One minor tweak, otherwise looks good.
pyproject.toml
Outdated
'greenlet < 3.0; python_version<"3.12"', | ||
'greenlet >= 3.0.0a1; python_version>="3.12.0rc0"', | ||
'importlib-metadata; python_version<"3.8"', | ||
"six", | ||
"werkzeug >= 0.14.1", |
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.
If we're going to specify a floor, we should move it inline with the Flask requirement so it's closer to what we're actually using:
"werkzeug >= 0.14.1", | |
"werkzeug >= 2.2.2", |
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.
@nateprewitt this may feel like the right thing, but since this is about the requirements of direct use by httpbin rather than Flask, the minimum version should match the direct use by httpbin not Flask, right? This will probably sound fabricated but maybe it still helps to illustrate the point: what if Flask adds backwards compatibility for Werkzeug 1.x.x in the future or what if Flask stops using Werkzeug altogether or starts using a fork of Werkzeug under a new name, say Werkzeug-ng? In all those cases "werkzeug >= 2.2.2" of httbin would then ask for more than Werkzeug needs to satisfy with regard to httbin. Does that make sense?
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.
the minimum version should match the direct use by httpbin not Flask, right?
Yep, I agree with that in principle for general software management. The problem is we haven't actively tested anything older than 2.2.2 with the current httpbin code. We can go back and manually validate 0.14.1 works, but given the test suite can't (reasonably) run that far back it seems disingenuous to provide that floor.
The version doesn't need to stay in sync with Flask, but it should reflect what we're confident about for a dependency. If we don't want the overhead of managing that, simply specifying werkzeug
as an unbounded dependency may be more accurate. That will be dictated by Flask for the foreseeable future and can be adjusted later if there is a legitimate floor to note in httpbin.
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.
@nateprewitt I'll speak my mind and then end on a practical note:
- I understand that fully supporting 0.14.1 in 2023 may not have the best value.
- I agree that minimum supported versions would ideally be covered, whatever it is.
- Simply specifying werkzeug as an unbounded dependency would allow more version of Werkzeug than "werkzeug >= 0.14.1" so that while simple that would not be safe.
I have put in >=2.2.2 now as suggested. Pushed.
.. as suggested by Nate Prewitt.
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.
Thanks @hartwork!
Fixes #38