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

add missing runtime dependencies #306

Merged
merged 6 commits into from
Mar 27, 2024
Merged

Conversation

vkunz
Copy link
Contributor

@vkunz vkunz commented Mar 19, 2024

hi,

flask-rebar uses bits from dependent modules without declaring the proper dependency.

Full disclosure: I am not super fluent in python, so this might not be the python way of fixing this.

Only commit f73264e is strictly necessary, everything else should be considered rfc.

I started a fresh flask/flask-rebar project. Running the built-in development server of flask complained about imports from missing modules.

BR,
Valentin

Flask-rebar uses type classes not available in all supported python
versions.

Missing type classes are imported from typing-extensions.

Signed-off-by: Valentin Kunz <[email protected]>
Type class ParamSpec was made available with python3.10.

Limit polyfill import to python versions missing ParamSpec.

Signed-off-by: Valentin Kunz <[email protected]>
Polyfills within typing-extensions dependency are only used within
specific python versions.

Limit the dependency to those specific versions.

Signed-off-by: Valentin Kunz <[email protected]>
Bits from this module are used directly within flask-rebar.

Signed-off-by: Valentin Kunz <[email protected]>
Copy link
Contributor

@VerinaG VerinaG left a comment

Choose a reason for hiding this comment

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

LGTM. @nlawrence22 any concerns?

@nlawrence22
Copy link
Contributor

@vkunz -- Sorry for the delayed response, thanks for the contribution, we really appreciate it! Your changes look good to me, but unfortunately It looks like tests are failing for issues unrelated to your changes -- I'd like for us to fix those underlying issues and ensure that those tests pass, and then rebase your changes on top of those fixes and make sure they pass those tests as well, if that's ok with you?

We should be able to get those fixes in by (and likely on) Friday, March 26th Pacific Standard Time.

@vkunz
Copy link
Contributor Author

vkunz commented Mar 27, 2024

@vkunz -- Sorry for the delayed response, thanks for the contribution, we really appreciate it!

no worries

Your changes look good to me, but unfortunately It looks like tests are failing for issues unrelated to your changes -- I'd like for us to fix those underlying issues and ensure that those tests pass, and then rebase your changes on top of those fixes and make sure they pass those tests as well, if that's ok with you?

sure, that's okay

We should be able to get those fixes in by (and likely on) Friday, March 26th Pacific Standard Time.

I have a locally working fix here I could submit but only if you'd like me to.

I wouldn't want to step on anybodies toes.

BR,
Valentin

@nlawrence22
Copy link
Contributor

@vkunz -- thanks for offering a second fix! No worries about stepping on any toes, contributions are always welcome.

It looks like Verina was able to get those changes in already, and your changes are now passing, so we'll go ahead and merge this PR. Thank you again for the contribution!

@nlawrence22 nlawrence22 merged commit 964057b into plangrid:master Mar 27, 2024
46 checks passed
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