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

ENH: Improve linting via GitHub Actions #496

Closed
2 of 4 tasks
obscurerichard opened this issue May 10, 2024 · 4 comments
Closed
2 of 4 tasks

ENH: Improve linting via GitHub Actions #496

obscurerichard opened this issue May 10, 2024 · 4 comments
Labels
feature-request a request for a new feature

Comments

@obscurerichard
Copy link

obscurerichard commented May 10, 2024

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, and flake8) 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 for black, 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

  • I agree to follow this project's Code of Conduct
@lwasser
Copy link
Collaborator

lwasser commented May 11, 2024

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.
we also are using RUFF which covers isort, black and many other linters and is fast given it's written in rust. so these CI checks are in place. if you can clarify the issues that you are trying to address in your current PR that would be helpful.

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!

@obscurerichard
Copy link
Author

Ah, that makes sense. So we are already covered via ruff and pre-commit.ci for the basics. I'll amend this issue and the PR to be more focused and omit changing things for linters that are already working well. However, it's interesting that the isort linting that the GitHub action

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 pre-commit.ci, which is also included in the original PR.

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.

@lwasser
Copy link
Collaborator

lwasser commented May 14, 2024

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!

@lwasser
Copy link
Collaborator

lwasser commented Jun 1, 2024

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

@lwasser lwasser closed this as completed Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request a request for a new feature
Projects
None yet
Development

No branches or pull requests

2 participants