FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

first sketch of cython-lint workflow by fchapoton · Pull Request #204 · flintlib/python-flint · GitHub

  1. flintlib
  2. python-flint

Repository navigation

first sketch of cython-lint workflow - #204

Merged
oscarbenjamin merged 5 commits into
flintlib:masterfrom
fchapoton:cython-lint
Aug 30, 2024
ConversationCommits (5)ChecksFiles changed
Merged

first sketch of cython-lint workflow#204
oscarbenjamin merged 5 commits into
flintlib:masterfrom
fchapoton:cython-lint

Conversation

fchapoton commented Aug 30, 2024

Copy link
Copy Markdown
Contributor

let's try to add a cython-lint checker in a workflow

oscarbenjamin commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

It might be that the workflow doesn't run until it is actually merged or something.

fchapoton commented Aug 30, 2024

Copy link
Copy Markdown
Contributor Author

yes, could be. I have to add many ignored codes too

oscarbenjamin reviewed Aug 30, 2024
View reviewed changes
Comment thread .github/workflows/lint.yml Outdated
runs-on: ubuntu-latest
strategy:
matrix:
python-version: ["3.9", "3.12"]

oscarbenjamin Aug 30, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not sure it matters but minimum version is 3.10 now.

Does the lint check depend on the Python version?

fchapoton Aug 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ok. I think the cython-lint does not, but maybe ruff checks will depend if they are added later. So let us keep a set of python versions

fchapoton Aug 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

now the lint workflow is running ; probably it will fail, let's see

fchapoton commented Aug 30, 2024

Copy link
Copy Markdown
Contributor Author

now with large-scale removal of unused variables and imports. The former in particular may benefit from a careful check.

oscarbenjamin reviewed Aug 30, 2024
View reviewed changes
Comment thread .github/workflows/lint.yml Outdated

- name: cython-lint
run: |
cython-lint --ignore=E114,E117,E127,E128,E129,E202,E221,E222,E231,E261,E262,E265,E302,E303,E306,E501,E701,E703,E711,E722,E731,E741,E743,W291,W293,W391,W605 src/

oscarbenjamin Aug 30, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does it still need all of these?

I think it is fine if there are lots of ignore code for now because they can be disabled incrementally.

It would be better if the ignore codes are in pyproject.toml though so that it works the same when running locally as in CI.

oscarbenjamin Aug 30, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Or maybe it is better to leave these here for now until most errors are fixed since we don't intend to keep all of these ignores codes.

fchapoton Aug 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think I only added codes that I have seen failing.

I have moved the config to pyproject.toml

fchapoton Aug 30, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Although my branch here is not based on my recent fix-ups, that in particular fix W605.

oscarbenjamin commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

I've been through all the diff and it looks good although I'll let the CI finish.

fchapoton commented Aug 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I have rebased the branch on master, squashed the first few commits and shortened the ignore list

oscarbenjamin commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

Okay, looks good to me. I'll wait for CI to finish.

oscarbenjamin commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

I'm going to add a development workflow page to the docs and I'll mention how to run cython-lint there. We should probably arrange it so that you can do e.g.

$ spin lint

since spin is the developer frontend.

oscarbenjamin commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

Okay this looks good. Thanks!

oscarbenjamin merged commit 54beefc into flintlib:master Aug 30, 2024
fchapoton deleted the cython-lint branch August 31, 2024 06:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

oscarbenjamin oscarbenjamin left review comments

Assignees

No one assigned

Labels

type: workflow

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL