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

Remove dependency upper-caps and update #340

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

pvandyken
Copy link
Contributor

Remove cached-property dep since functionality is provided by functools in py38

Remove all upper version limits in main dependency block. Where kept, justification comment is added

Upgraded pvandyken-deprecated dependency, which packages proper typing support (thus, remove our own typings file)

@pvandyken pvandyken added the maintenance Updates or improvements that do not change functionality of the code label Sep 21, 2023
Copy link
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

Largely lgtm code-wise (just realized the one test is failing for py3.11), with a very minor comment (that upon second thought doesn't really require any action given these are more dev dependencies than for distribution).

Aside from cookiecutter (which will get replaced in #336 anyway), is there opposition to changing the ^ to a >=?. The caret requirements would be placing an upper-cap on the dependencies if I am interpreting the poetry docs correctly. Making the change would also bring us closer to following PEP440.

@pvandyken
Copy link
Contributor Author

Aside from cookiecutter (which will get replaced in https://github.com/akhanf/snakebids/pull/336 anyway), is there opposition to changing the ^ to a >=?. The caret requirements would be placing an upper-cap on the dependencies if I am interpreting the poetry docs correctly. Making the change would also bring us closer to following PEP440.

For a specific dependency, or for everything? And are you asking about removing the ^ from a syntax perspective (and replacing with >=x,<y), or just on removing upper limits?

@pvandyken
Copy link
Contributor Author

The test failure should be fixed by a commit I made in #336, so I'll wait for that one to merge, then deal with the failure here

@kaitj
Copy link
Contributor

kaitj commented Sep 21, 2023

Aside from cookiecutter (which will get replaced in #336 anyway), is there opposition to changing the ^ to a >=?. The caret requirements would be placing an upper-cap on the dependencies if I am interpreting the poetry docs correctly. Making the change would also bring us closer to following PEP440.

For a specific dependency, or for everything? And are you asking about removing the ^ from a syntax perspective (and replacing with >=x,<y), or just on removing upper limits?

More a general thing and yeah, replacing ^ with >=x (and <y where it is needed)

Copy link
Contributor

@tkkuehn tkkuehn left a comment

Choose a reason for hiding this comment

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

Pending tests passing this looks good

@pvandyken pvandyken force-pushed the maint/dep-updates branch 2 times, most recently from bef3139 to 3cffa9c Compare November 29, 2023 18:50
@pvandyken
Copy link
Contributor Author

I'm going to keep the ^ syntax for the dev dependencies, just because it's more concise and imo upper limits are at least not harmful for dev dependencies. Pending the test passing I'm going to merge

Remove cached-property dep since functionality is provided by functools
in py38

Remove all upper version limits in main dependency block. Where kept,
justification comment is added

Upgraded pvandyken-deprecated dependency, which packages proper typing
support (thus, remove our own typings file)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
A recent hypothesis release by default loads a directory based database
of examples. This is incompatible with pyfakefs as hypothesis is unable
to access the file system. Until a full workaround can be designed,
we'll need to use the inmemory database
@pvandyken pvandyken merged commit 0b89e10 into khanlab:main Nov 29, 2023
28 checks passed
@pvandyken pvandyken deleted the maint/dep-updates branch November 29, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Updates or improvements that do not change functionality of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants