Skip to content

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Feb 29, 2024

Changes proposed in this pull request

ruff check --select G

Documentation that should be reviewed

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.50%. Comparing base (5bb1cf5) to head (feeec8f).
⚠️ Report is 93 commits behind head on master.

Files with missing lines Patch % Lines
fmriprep/_warnings.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3242   +/-   ##
=======================================
  Coverage   73.50%   73.50%           
=======================================
  Files          60       60           
  Lines        4582     4582           
  Branches      585      585           
=======================================
  Hits         3368     3368           
  Misses       1085     1085           
  Partials      129      129           

☔ 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.

@effigies
Copy link
Member

effigies commented Mar 4, 2024

I'm not sure why we should care about this.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Mar 4, 2024

The linter recommends that because that's what many programmers suggest: defer string interpolation to the log function so that string interpolation doesn't occur whenever the log function doesn't print at the current log level. So that would be a performance issue. The rationale is that the speed gain when deferring string interpolation with terse logging outwheighs the speed gain of f-strings with verbose logging. Other programmers don't agree.

@effigies
Copy link
Member

We decided in #3515 to generally prefer f-strings. Thanks for the effort, but we won't be enabling this rule and I'm closing this.

@DimitriPapadopoulos
Copy link
Contributor Author

That's not really about- f-strings.

Just to make sure, this is not about using

logger.info("info: %s" % s)

vs.

logger.info(f"info: {s}")

It's about

logger.info("info: %s", s)

@effigies
Copy link
Member

Correct.

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.

2 participants