-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Discussion: Aligning Python Code Formatting with PEP8 vs. Readability Preferences #45706
Comments
cms-bot internal usage |
A new Issue was created by @Benedikttk. @Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
So as food for thought, the very beginning of PEP 8 says when NOT to follow its convention
|
So looking at examples in the PEP, it looks like the formatter is not actually obeying the rules. In the examples in the PEP it has # Correct:
x = 1
y = 2
long_variable = 3 and explicitly says
which the examples you gave from this formatter does not do. |
assign core |
New categories assigned: core @Dr15Jones,@makortel,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks |
PEP8 lists an exception to that later (or, they don't count function arguments as "assignment", or something)
https://peps.python.org/pep-0008/#other-recommendations Personally I would prefer to have spaces around |
I suppose we need to tag @cms-sw/all-l2 to gather feedback. |
+1 |
Both examples of PEP8 formatting are hideous. |
this:
is actually missing the closing process.MessageLogger = cms.Service("MessageLogger",
cerr = cms.untracked.PSet(
enable = cms.untracked.bool(False)
),
debugModules = cms.untracked.vstring('*'),
files = cms.untracked.PSet(
readDBdebug = cms.untracked.PSet(
DEBUG = cms.untracked.PSet(
limit = cms.untracked.int32(-1)
),
INFO = cms.untracked.PSet(
limit = cms.untracked.int32(-1)
),
extension = cms.untracked.string('.out'),
noLineBreaks = cms.untracked.bool(True),
threshold = cms.untracked.string('DEBUG')
),
readDBerrors = cms.untracked.PSet(
extension = cms.untracked.string('.out'),
threshold = cms.untracked.string('ERROR')
)
)
) |
This:
I understand the rationale, however most cases will have parameters of very different lengths, making it difficult to apply. I guess if we want to have consistent rules, we would go with TOFRMapH = cms.PSet(
Nxbins = cms.int32(1200),
xmin = cms.double(0.),
xmax = cms.double(120.),
Nybins = cms.int32(100),
ymin = cms.double(0.),
ymax = cms.double(50.),
switch = cms.bool(False)
) |
Some more comments...
|
An empty parameter = cms.PSet() A parameter = cms.PSet(
flag = cms.bool(False),
answer = cms.int32(42)
) What about a parameter = cms.PSet(
nice = cms.bool(True)
) but I've seen in many place all on a single line: parameter = cms.PSet( ugly = cms.bool(True) ) though I'm not sure about the whitespaces. |
|
If I am not wrong , there are few thing which we can configure and also we can enable/disble some of the pycodestyle https://pycodestyle.pycqa.org/en/latest/intro.html#configuration |
At least here PEP8 seems to agree to have the trailing comma, except when placed on a single line with the closing delimeter
https://peps.python.org/pep-0008/#when-to-use-trailing-commas |
another (currently more popular) formatter option is https://github.com/psf/black |
Yes hello, Ruff enforces a limit of 88 characters, this is configurable via the line-length setting in the ruff.toml file. |
Yes, black is also very good, especially for solo purposes. We did look into black, although Ruff is supposedly faster in large repositories than its counterparts Black, so due to that and other things we went with Ruff. Ruff: Linting and Formatting |
(I missed Ruff at the top - i mean to refer to pycodestyle..) |
As previously discussed in Core Software meeting on 30/Jul/2024 (indico link), we would like to address the issue of the Python formatting style. The tool we are using for formatting is Ruff and it enforces PEP8 formatting.
We have seen some pieces of code formatted by the authors for readability in a specific way. For example, in Validation/SiTrackerPhase2V/python/Phase2TrackerValidateDigi_cff.py:
In this case, Ruff reports that all those white spaces do not comply with PEP8 and they will be automatically changed to:
We can find another example of readability vs formatting in GeometryReaders/XMLIdealGeometryESSource/test/testReadXMLFromDB.py, where the author aligned the input parameters as follows:
This will be reformated to:
Some people prefer to maintain readability in their code, which may not always align with PEP8. Currently, it is not possible to selectively retain or discard specific formatting elements to accommodate individual coding styles. So we can either format all white spaces according to PEP8 or none.
We are opening this issue seek your feedback on this. I am open to suggestions to progressively format the Python code in CMSSW without disturbing the contributors.
Thank you
The text was updated successfully, but these errors were encountered: