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

Support millisecond intervals #76

Closed
wants to merge 4 commits into from

Conversation

judahrand
Copy link
Contributor

@judahrand judahrand commented Sep 23, 2022

This is pretty straightforward and allows sub-second intervals to be specified like any other. Mostly type hinting changes.

Closes: #75

@judahrand
Copy link
Contributor Author

@vutran1710 This seems like a CI issue rather than a problem with my changes?

@JWCook
Copy link
Contributor

JWCook commented Sep 23, 2022

Yeah, that appears to be an issue with poetry 1.2 dropping support for python 3.6:
snok/install-poetry#89

That doesn't have anything to do with your changes, but the solution would be to either revert to poetry 1.1, or drop support for python 3.6. I made a separate PR for this: #77

@JWCook JWCook mentioned this pull request Sep 23, 2022
@vutran1710
Copy link
Owner

@judahrand you are gonna need to bump your version

@judahrand
Copy link
Contributor Author

@judahrand you are gonna need to bump your version

This should be find now given that it was updated on master with the Python 3.6 drop?

@judahrand
Copy link
Contributor Author

@JWCook CI still seems broken?

@JWCook
Copy link
Contributor

JWCook commented Sep 25, 2022

Hmm, the master branch build succeeded, and this looks like a different error:

/home/runner/work/PyrateLimiter/PyrateLimiter/.venv/bin/python: bad interpreter: No such file or directory

Somehow the virtualenv's interpreter is missing? I don't think I've seen that one before. It must be a problem with the GitHub Actions cache. The cache key is defined here:

key: venv-${{ matrix.python-version }}-latest-${{ hashFiles('poetry.lock') }}

Probably the easiest way to invalidate it is to run poetry update and commit the changes. That will give poetry.lock a different hash, which will create a different cache key, which will cause the next CI run to create a new virtualenv instead of using the cached virtualenv.

@judahrand
Copy link
Contributor Author

@JWCook That's sorted it!

@codecov
Copy link

codecov bot commented Sep 25, 2022

Codecov Report

Base: 98.15% // Head: 98.16% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (b69561b) compared to base (ca1a54b).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head b69561b differs from pull request most recent head 37da4d7. Consider uploading reports for the commit 37da4d7 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #76   +/-   ##
=======================================
  Coverage   98.15%   98.16%           
=======================================
  Files           7        7           
  Lines         379      381    +2     
  Branches       33       33           
=======================================
+ Hits          372      374    +2     
  Misses          4        4           
  Partials        3        3           
Impacted Files Coverage Δ
pyrate_limiter/constants.py 100.00% <100.00%> (ø)
pyrate_limiter/request_rate.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@judahrand judahrand force-pushed the millisecond branch 2 times, most recently from 6bd9e8b to 37da4d7 Compare September 25, 2022 20:31
@judahrand
Copy link
Contributor Author

@JWCook This did pass all the tests...

There's definitely something funky with the CI on this project...

@JWCook
Copy link
Contributor

JWCook commented Sep 25, 2022

Yeah, the tests all passed at least once, so this should be fine to merge.

@vutran1710 It seems like the build is now failing whenever it uses a cached virtualenv, which is something I haven't seen before. Unless you happen to know what's wrong there, it might be best just to disable the cache for now?

@vutran1710
Copy link
Owner

@judahrand you need to bump version in pyproject.toml and resolve the conflicts

@judahrand
Copy link
Contributor Author

judahrand commented Sep 26, 2022

@judahrand you need to bump version in pyproject.toml and resolve the conflicts

Are you saying you want a minor version bump?

It strikes me as super weird that this is decided/managed by the contributors rather than part of the release process or managed automatically with a semver tool (and still determined at release time).

@vutran1710
Copy link
Owner

@judahrand you need to bump version in pyproject.toml and resolve the conflicts

Are you saying you want a minor version bump?

It strikes me as super weird that this is decided/managed by the contributors rather than part of the release process or managed automatically with a semver tool (and still determined at release time).

Yes, it should be automatically using some semver tool (I created an issuse for that), but have yet have time to implement it, thats why manual work is still needed for now. sorry for the inconvenience.

@vutran1710
Copy link
Owner

once the PR has been merged, the release will be done automatically, thats why i need version bump in every pr.

@JWCook
Copy link
Contributor

JWCook commented Sep 28, 2022

#79 is at least a temporary fix for the cache issue encountered here.

Yes, it should be automatically using some semver tool (I created an issuse for that), but have yet have time to implement it

Meanwhile, another option to consider is releasing only on git tags instead of on every push to master. That makes it easier to bundle multiple PRs into one release, or skip releases for minor things like config changes.

@judahrand
Copy link
Contributor Author

#79 is at least a temporary fix for the cache issue encountered here.

Yes, it should be automatically using some semver tool (I created an issuse for that), but have yet have time to implement it

Meanwhile, another option to consider is releasing only on git tags instead of on every push to master. That makes it easier to bundle multiple PRs into one release, or skip releases for minor things like config changes.

Honestly, this is way more sensible imo

@JWCook
Copy link
Contributor

JWCook commented Sep 29, 2022

Alright, can you rebase onto master again, and update the version once more to 2.8.3? Hopefully that's the last thing.

@vutran1710 vutran1710 closed this Oct 14, 2022
@vutran1710
Copy link
Owner

No activity

@judahrand
Copy link
Contributor Author

You don't want to add that single commit/rebase yourself and merge? Or leave it open and give me the opportunity to get back to it?!

Rude.

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.

Support sub second intervals
3 participants