Skip to content

Fix all logging to use %-interpolation and not .format, sort imports in touched files, add pylint-ing for % formatting in log messages to tox -e lint - #7118

Merged
bpoldrack merged 4 commits into
datalad:maintfrom
yarikoptic:enh-pylint
Nov 2, 2022

Conversation

@yarikoptic

@yarikoptic yarikoptic commented Oct 25, 2022

Copy link
Copy Markdown
Member

#7117 made me look into tightening our automated linting. Adding pylinting for a single warning/error to start with:

W1202: Use lazy % or % formatting in logging functions (logging-format-interpolation)

which I believe nobody would doubt to be a good thing to do.

[appveyor skip]
[travis skip]

edit: had to cancel appveyor manually -- for some reason it ignored the annotation I gave

@yarikoptic yarikoptic added the semver-internal Changes only affect the internal API label Oct 25, 2022
@jwodder

jwodder commented Oct 26, 2022

Copy link
Copy Markdown
Contributor

@yarikoptic I've fixed all occurrences of the W1202 lint.

@yarikoptic yarikoptic changed the title Add pylint-ing for % formatting in log messages to tox -e lint Fix all logging to use %-interpolation and not .format, sort imports in touched files, add pylint-ing for % formatting in log messages to tox -e lint Oct 31, 2022
@yarikoptic yarikoptic added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Oct 31, 2022
yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Oct 31, 2022
@yarikoptic

Copy link
Copy Markdown
Member Author

travis -- filed #7126 and restarted , appveyor -- looked like some ssh issue

[gw0] PASSED ../datalad/core/local/tests/test_save.py::test_add_mimetypes 
1185../datalad/core/local/tests/test_save.py::test_gh1597 channel 22: open failed: connect failed: open failed
1186channel 22: open failed: connect failed: open failed
1187channel 22: open failed: connect failed: open failed
1188channel 22: open failed: connect failed: open failed
1189channel 22: open failed: connect failed: open failed
1190channel 22: open failed: connect failed: open failed
1191

restarted

@codecov

codecov Bot commented Oct 31, 2022

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.33028% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.93%. Comparing base (da44dbc) to head (e34ab44).
Report is 1135 commits behind head on maint.

Files with missing lines Patch % Lines
datalad/support/archives.py 88.23% 2 Missing ⚠️
datalad/support/annexrepo.py 90.90% 1 Missing ⚠️
datalad/utils.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7118      +/-   ##
==========================================
+ Coverage   89.37%   90.93%   +1.56%     
==========================================
  Files         355      355              
  Lines       46498    46506       +8     
  Branches     6327     6327              
==========================================
+ Hits        41559    42292     +733     
+ Misses       4924     4199     -725     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

yarikoptic and others added 3 commits October 31, 2022 12:27
For the W1202: Use lazy % or % formatting in logging functions (logging-format-interpolation)
which I believe nobody would doubt to be a good thing to do

[appveyor skip]
[travis skip]
Use lazy % or % formatting in logging functions (logging-format-interpolation)
@github-actions github-actions Bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Oct 31, 2022

@bpoldrack bpoldrack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally, looks good to me.
But why maint? Seems to me it would be easier to resolve in master (for example metadata removal).

@yarikoptic

yarikoptic commented Nov 1, 2022

Copy link
Copy Markdown
Member Author

Generally, looks good to me. But why maint? Seems to me it would be easier to resolve in master (for example metadata removal).

because it is a bug to use f-strings in logging. Most likely most of them are benign and should never result in triggering some condition which would lead to datalad errorring out but we don't know. This actually gives me an idea to add some %d into obscure filename -- might be "fun" ;) edit: not needed -- we already have %b5 as one of the possible parts of the most obscure filename:

❯ python -c 'from datalad.tests.utils_pytest import OBSCURE_FILENAME; print(OBSCURE_FILENAME)'
 |;&%b5{}'"<>ΔЙקم๗あ .datc 

so if there was a log line which had this already, might have triggered that condition.

@yarikoptic

Copy link
Copy Markdown
Member Author

for example metadata removal

BTW IMHO it is a bit misleading to name it "removal" -- it is a "move to -deprecated" so not really "fire and forget", and indeed it would cause conflicts and then lack of similar fix in deprecated since most likely people just would not care to fix it there, although that code would still be there and might even be used by some. But that is ok

@yarikoptic

Copy link
Copy Markdown
Member Author

another reason -- to avoid lingering merge conflicts from maint to master on any PR which touches those log lines...

anyways -- if no strong objections, I would prefer to merge tomorrow and proceed with preparing merge into master.

@bpoldrack

Copy link
Copy Markdown
Member

Ok, let's proceed then.

@bpoldrack
bpoldrack merged commit f570bcd into datalad:maint Nov 2, 2022
@yarikoptic-gitmate

Copy link
Copy Markdown
Collaborator

PR released in 0.17.9

@datalad datalad deleted a comment from yarikoptic-gitmate Nov 7, 2022
@yarikoptic
yarikoptic deleted the enh-pylint branch January 20, 2023 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-internal Changes only affect the internal API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants