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

fix: loosen restriction on the limits dependency #157

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Conversation

sanders41
Copy link
Collaborator

Closes #156

This is one possible solution to #156 if you want to keep supporting limits <3.0. The current setting caps limits at 2.x, this change will allow anything >= 2.3. All the tests are passing so my assumption is none of the breaking changes in 3.0 cause issues here.

@andreagrandi
Copy link
Collaborator

Thanks @sanders41 ! I hope it gets merged and released 🙏🏻

@laurentS laurentS changed the title Loosen restriction on the limits dependency fix: loosen restriction on the limits dependency Apr 7, 2023
@laurentS
Copy link
Owner

laurentS commented Apr 7, 2023

Hi @sanders41 thanks for opening this. It looks ok to me, particularly seeing that the CI checks used limits == 3.3.1.
@andreagrandi I'm merging now, please do report back if the new master branch is failing in any way for you.

@laurentS laurentS merged commit d7b8608 into master Apr 7, 2023
@laurentS laurentS deleted the limits branch April 7, 2023 11:34
@andreagrandi
Copy link
Collaborator

Hi @sanders41 thanks for opening this. It looks ok to me, particularly seeing that the CI checks used limits == 3.3.1. @andreagrandi I'm merging now, please do report back if the new master branch is failing in any way for you.

hi @laurentS first of all thank you so much for addressing this so quickly ❤️

I temporary changed my requirements.txt like this:

# slowapi==0.1.7
git+https://github.com/laurentS/slowapi@master

and rerun our tests and I confirm I'm not getting that error anymore 🎉

It's worth noticing two things:

  1. the error I was getting:
    warnings.warn("pkg_resources is deprecated as an API", DeprecationWarning)
E   DeprecationWarning: pkg_resources is deprecated as an API

was enough, in my case to make nox fail the test, but if I ran the tests with pytest directly, the warning wasn't "enough" to make the test fail.

  1. A colleague of mine, tried in all possible ways (we replicated the same packages, same environment, same Python version etc....) to replicate the issue, but she wasn't able to replicate: on her machine she was indeed getting warnings.warn("pkg_resources is deprecated as an API", DeprecationWarning) but this warning wasn't enough to make nox fail. It's just like our systems had two different levels of failure 🤷🏻‍♂️

p.s: I'm looking forward to switch to the new version once released, in the mean time I will keep using the master branch version locally.

Thanks again 🙏🏻

@sanders41
Copy link
Collaborator Author

@andreagrandi I am guessing this isn't the reason, but any chance you have the environment variable PYTHONWARNINGS set to error? https://docs.python.org/3/using/cmdline.html#envvar-PYTHONWARNINGS

@andreagrandi
Copy link
Collaborator

@andreagrandi I am guessing this isn't the reason, but any chance you have the environment variable PYTHONWARNINGS set to error? https://docs.python.org/3/using/cmdline.html#envvar-PYTHONWARNINGS

if I do env | grep PYTHONWARNINGS it gives me nothing

@sanders41
Copy link
Collaborator Author

Your result is the same as mine. I figured it was a long shot.

@laurentS
Copy link
Owner

laurentS commented Apr 7, 2023

Shameless call for help, I've opened #109 a few days ago to try and fix the blocking point around releases. If anyone has time to take a look, it'll be helpful.

@andreagrandi I don't know where your error comes from, I've never used nox directly. My only guess would be to check if nox isn't somehow using a "user level" config file, something like ~/.nox/config or maybe you have a shell alias that replaces nox with nox --some-stricter-option?
Side note, feel free to open a release PR along the lines of #120 for v0.1.8, that should get you contributor access to the repo as per #58 :)

@andreagrandi
Copy link
Collaborator

Side note, feel free to open a release PR along the lines of #120 for v0.1.8, that should get you contributor access to the repo as per #58 :)

I can definitely give it a try. My only blocker is that I never used poetry so I'm not sure how to regenerate that lock file, but I can try

@laurentS
Copy link
Owner

laurentS commented Apr 7, 2023

No pressure :)

I can definitely give it a try. My only blocker is that I never used poetry so I'm not sure how to regenerate that lock file, but I can try

poetry lock will generate the lockfile, poetry install will do the same AND install the required dependencies. https://python-poetry.org/docs/cli/#lock for more details.

@andreagrandi
Copy link
Collaborator

I prepared this #159 but I still need to take care of poetry.lock

@andreagrandi
Copy link
Collaborator

➜  slowapi git:(prepare-release-0-1-8) poetry lock
Updating dependencies
Resolving dependencies... (4.0s)
➜  slowapi git:(prepare-release-0-1-8) git status
On branch prepare-release-0-1-8
Your branch is up to date with 'origin/prepare-release-0-1-8'.

nothing to commit, working tree clean

uhm

@sanders41
Copy link
Collaborator Author

I ran the lock to make sure limits was updated for testing so I'm guessing this is why you aren't getting any new updates.

@andreagrandi
Copy link
Collaborator

Perfect then! Please double check the changelog file, because I generated it by reading meaningful (imho) commit messages that were done after the last release. Thanks

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.

slowapi uses limits==2.8.0 which contains usage of deprecated pkg_resources
3 participants