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

pep8 strict formating #2605

Merged
merged 65 commits into from
Jan 25, 2025
Merged

pep8 strict formating #2605

merged 65 commits into from
Jan 25, 2025

Conversation

un-pogaz
Copy link
Contributor

@un-pogaz un-pogaz commented Jan 13, 2025

Following the discussion on mobileread, here the pull request for a global and strict pep8 formating.

If you have any concern on a commit, I can easly edit them.

(forced push for fix some test fail)

@un-pogaz un-pogaz force-pushed the ruff-pep8-strict branch 3 times, most recently from 2d3ed15 to cc7a4dc Compare January 13, 2025 13:06
@kovidgoyal
Copy link
Owner

Please resolve the conflicts and rebase on master

@un-pogaz
Copy link
Contributor Author

done

@kovidgoyal
Copy link
Owner

Also this is rather too much code for a human being to review. It means I cant be sure you didnt sneak in some malicious code in there (Not that I think you would just that I cant review it to check). SO maybe instead split it up into one PR that adds the changes to check.py and the strict.tmol file. Then let me know what command I need to run to generate the automatic changes. I will run and commit them myself. Then if there are some manual changes left in this PR I can review and merge them without all the noise form the automatic changes.

@un-pogaz
Copy link
Contributor Author

Oh, understandable. I will make this light PR. Do you prefer discute of how decompose and reproduct it here, on the light PR or on mobileread?

@eli-schwartz
Copy link
Contributor

What I usually do in such a case is include in the commit message, something like:

produced by running:

ruff check --select XXXXX --fix

and committing the results

Given a per-commit reproducer command it is quite easy with only light bisection to verify the results are as expected.

@un-pogaz
Copy link
Contributor Author

un-pogaz commented Jan 13, 2025

The Ruff code/rules performed are include in each commit. Nota that it will not allway fully match with the reference commit because I slightly twearked the result to better match the calibre format, or even manualy add somme edit to extend the formating because Ruff don't catch them.
(this was especially visible for the blank-line because the system of region used in calibre is pourly compatible with the auto-fix)

Some commit have none comment because is was perform entierly manualy.

The "various whitespace" has a section 'partial' wich is various formating that have apply very selectively, because a full auto-fix will realy break the calibre formating in the wrong way.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Jan 13, 2025

Then those won't be doable via setup.py check --strict either? I'm not talking about those changes. :) Make them in a second, followup commit where it is therefore possible to produce a preliminary commit that can be machine-validated.

@un-pogaz
Copy link
Contributor Author

No there are not doable by setup.py check --strict and... it's a good idea.

It's will be a bit a tidious to do to separate the "machine auto-fix" from "manual edit", but if that can facilite the review, I start that tomorow.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jan 14, 2025 via email

ruff 'E302,E303,E304,E305,W391'
ruff 'E111,E112,E113,E117'
ruff 'E114,E115,E116,E261,E262,E265'
ruff 'E114,E115,E116,E261,E262,E265'
ruff 'E201,E202,E211,E251,E275'
!partial 'E203,E222,E241,E271,E272'
@un-pogaz
Copy link
Contributor Author

Okay, here a new version of the pull request, following the idea of separate the change into several commits depending of the type of change. The type of commit are:

  • auto-fix: change was perfom with the --fix-only command of Ruff
  • manual: error was fixed manualy, following the errors reported by Ruff
  • extra-edit: additional edit, generaly to extend some thing undetected by Ruff

Inside the body of the commit, there is the rule code targeted by Ruff, plus aditional command if so. !partial keyword indicated rule used as rereference for this edit, but not fully included into ruff-strict-pep8.toml because a strict application will go againt "calibre standard".

Note: "add/remove blank-line" commit was marqued 'extra-edit' because is a complet mess of intricate auto-fix, manual and extra-edit, and separet them will be a even more horrible mess to review. For a programmatic review of this commit, I suggest to compare the files before/after line by line, minus empty lines, which should theoretically result at the same array since this commit only edit empty lines.

@kovidgoyal kovidgoyal merged commit 7e61ea2 into kovidgoyal:master Jan 25, 2025
4 checks passed
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.

3 participants