-
Notifications
You must be signed in to change notification settings - Fork 202
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
ENH: Improve linting via GitHub Actions #496
Comments
hey @obscurerichard - thanks for the issue. We are using the pre-commit.ci bot to lint every pr. it actually will do more than just check - it will fix in place if we need it to. you can check out our development docs here: https://stravalib.readthedocs.io/en/latest/contributing/development-guide.html#pre-commit-ci-bot . i do see that we may need to update the docs to mention ruff as a way to call all of those other linters but pre-commit is mentioned there! |
Ah, that makes sense. So we are already covered via I do see that there is some linting done via GitHub Actions, and there is one new linter I'd love to apply to this, FawltyDeps - which can find undeclared or unused dependencies. It did find a couple undeclared transitive dependencies in StravaLib. Full disclosure: I work for the firm that created FawltyDeps and have used this library as a test case for the new GitHub action I just wrote - but I do really rely on Stravalib and I do think this particular linter could be valuable. However it could also be applied via I'm going to take another pass at this and look at the docs more closely and see if there are any doc improvements I can make while I'm at it. |
hey @obscurerichard i am familiar with fawltydeps - i run pyOpenSci and it was actually submitted to us for review a while back. I don't think we need to add fawltydeps as a pre-commit hook in this case. Our infra doesn't change rapidly and as such i think if we run it once and it finds issues, that would be enough and less overhead on ci. i'm unclear however as to what missing dependencies it actually found. in the pr that you submitted, the pyproject.toml has some deps in incorrect locations. So it would be super helpful if you could be very clear here in this issue as to what your goals are and what is incorrect prior to submitting a pr! that would help us understand what actually needs to change. i'll leave a few comments in the open pr but i do not think we should merge that pr as is - especially without understanding the intent. NOTE: your pr did make me realize when we switched to ruff, we didn't update the docs to reflect that! so that would be a helpful update to our docs if you are inclined to fix the docs! |
i am going to close this issue given we closed the pr associated with it. Thank you again @obscurerichard for interacting with us here!! we can always open another issue to update our docs |
Feature Type
Adding new functionality to stravalib
Changing existing functionality in stravalib
Removing existing functionality in stravalib
Problem Description
Several of the linters mentioned in the documentation (
black
,isort
, andflake8
) currently do not have any CI checks enforced via GitHub actions.mypy
type checking is enforced via GItHub Actions CI, which is awesome.Feature Description
Add a
lint
GitHub action that runs linters forblack
,isort
,flake8
, and possibly FawltyDeps. Lay the groundwork for adding other linters.Alternative Solutions
Add any new linters via https://github.com/stravalib/stravalib/blob/main/.pre-commit-config.yaml as well.
Additional Context
I have created an implementation of this in https://github.com/obscurerichard/stravalib/tree/gh-actions-lint and intend to raise a PR. I'm the primary maintainer of Freezing Saddles - see https://github.com/freezing-saddles - perhaps the first system to use Stravalib (thanks @hozn !)
Code of Conduct
The text was updated successfully, but these errors were encountered: