Skip to content

378b ruff config update dbodor#397

Merged
DaniBodor merged 6 commits intomainfrom
378b-ruff-config-update-dbodor
Aug 2, 2024
Merged

378b ruff config update dbodor#397
DaniBodor merged 6 commits intomainfrom
378b-ruff-config-update-dbodor

Conversation

@DaniBodor
Copy link
Collaborator

@DaniBodor DaniBodor commented Jul 12, 2024 •
edited
Loading

Additional updates to ruff settings on top of #384.

Extra changes:

  • also perform ruff formatting check in githook and linting workflow
  • removed explicit excluded file types
    • and will be read from ruff defaults instead
  • output format is defaulted to "concise"
    • this used to be the default, but it changed to "full" as per ruff 0.5 and it's very lengthy.
  • use ruff docstring convention rather than explicitly setting which docstring rules to ignore
  • Turned on (almost) all linting rules, instead of hand-picking the main ones.
    • This may be slightly contentious, but I find that in a new project it's quite manageable (if applied to an existing project, it may be overwhelming)
    • Some rules are suppressed, which are the ones I have found to be over the top. I realize that this decision is somewhat arbitrary, but still think it works.
  • Updated actions/checkout and actions/setup-python to their newest versions
    • this is a bit of scope creep...

@sjvrijn sjvrijn mentioned this pull request Jul 29, 2024
Merged
2 tasks
Base automatically changed from 378-ruff-config-update to main July 30, 2024 10:15
@egpbos
Copy link
Collaborator

egpbos commented Jul 30, 2024

#384 merged, this can now be rebased on main.

@egpbos
Copy link
Collaborator

egpbos commented Jul 30, 2024

Damn, sorry for the sloppy review, but there is another ruff issue we missed in #384: in the build action, there is a linting workflow that also calls ruff .. That should become ruff check .. Can someone perhaps add a fix for that in this PR?

DaniBodor added 5 commits July 30, 2024 14:35
@DaniBodor @sjvrijn
perform ruff format check (in addition to linting) in workflow and in…
2f33fb9 
… pre-commit hook

also explicitly call `ruff check` rather than deprecated `ruff .`
@DaniBodor @sjvrijn
update deprecated format of ruff settings in pyproject.toml
1af168e
@DaniBodor @sjvrijn
use ruff docstring convention
ab81417 
more consistent than explicitly ignoring certain rules
@DaniBodor @sjvrijn
more aggressive linting checks
0c66450
@DaniBodor @sjvrijn
update checkout and setup-python versions
1360ccf 
for both current project as well as in cookiecutter
@sjvrijn sjvrijn force-pushed the 378b-ruff-config-update-dbodor branch from 8e47740 to 1360ccf Compare July 30, 2024 12:43
@sjvrijn
Copy link
Contributor

sjvrijn commented Jul 30, 2024

Damn, sorry for the sloppy review, but there is another ruff issue we missed in #384: in the build action, there is a linting workflow that also calls ruff .. That should become ruff check .. Can someone perhaps add a fix for that in this PR?

@egpbos As far as I can tell, that's already in this PR, see build.yml

@sjvrijn
Copy link
Contributor

sjvrijn commented Jul 30, 2024

@egpbos Issue was in the test, not the build.yml itself.
Test are passing now. I've made the choice to ignore the newly raised issues in tests and docs/conf.py, but to add type hints to the sample package file rather than ignoring the type annotation issues all together. I think that making type checking optional and/or deferring to another tool like mypy should be a separate issue

sjvrijn
sjvrijn approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@sjvrijn sjvrijn left a comment

Choose a reason for hiding this comment

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

LGTM

@DaniBodor
Copy link
Collaborator Author

DaniBodor commented Jul 31, 2024

@egpbos , can this be merged now?

@egpbos
Copy link
Collaborator

egpbos commented Jul 31, 2024

Reviewer approved, so go for it :)

@DaniBodor DaniBodor merged commit a50915a into main Aug 2, 2024
@DaniBodor DaniBodor deleted the 378b-ruff-config-update-dbodor branch August 2, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjvrijn sjvrijn sjvrijn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DaniBodor @egpbos @sjvrijn