mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard
verified publisher icon mne-tools

Fixes for #12360 Replacing percent format with f-strings

Open hasrat17 opened this issue 1 year ago • 4 comments

Reference issue

FIXES #12360

What does this implement/fix?

Replace percent format with format specifiers (f-strings)

Additional information

Used this command to capture percent format ruff check mne --config pyproject.toml --select UP031 Screenshot from 2024-02-27 02-04-43

hasrat17 avatar Feb 26 '24 21:02 hasrat17

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

welcome[bot] avatar Feb 26 '24 21:02 welcome[bot]

Thanks for your contribution, @hasrat17! There are numerous tests failing; this would need to be resolved before we can proceed here. Thanks!

hoechenberger avatar Feb 27 '24 08:02 hoechenberger

Hey @hoechenberger, This is my first contribution to open source; sorry for asking this, but can you please correct me if I am wrong? So I need to run this command pytest -m "not ultraslowtest" mne and check whether everything is passing. And the test case here, which is failing like this one, Tests / ubuntu-latest / pip-pre / 3.11 (pull_request) in details shows an extensive file, how to track the error from there to resolve it. image

hasrat17 avatar Feb 27 '24 18:02 hasrat17

Hello, I won't have time to assist you here, sorry. But I'm sure some of the other developers will chime in 👍

hoechenberger avatar Feb 28 '24 10:02 hoechenberger

And updating this branch to latest main should fix the pip-pre failures

larsoner avatar Mar 06 '24 20:03 larsoner

Hey @drammock @cbrnr can you please review and if everything good then merge this request.

hasrat17 avatar Mar 09 '24 03:03 hasrat17

The top of #12360 mentions:

Ruff rule UP031 can then be enabled in pyproject.toml.

can you try enabling it to see if it passes locally when you run pre-commit run --all ruff? If not you might need some # noqa: UP031 or similar on some lines if there are ones we want to keep as %-style.

larsoner avatar Mar 11 '24 16:03 larsoner

Hey @larsoner, I Enabled the Ruff rule UP031 and ran this pre-commit run --all ruff few tests failed (screenshot). Then I added # noqa: UP031 in some lines, and all tests passed. Screenshot from 2024-03-12 00-32-49

hasrat17 avatar Mar 11 '24 19:03 hasrat17

Then I added # noqa: UP031 in some lines, and all tests passed.

I think for at least the first two cases I looked at in 9ab9c76 an f-string would have been better than a # noqa line

larsoner avatar Mar 11 '24 21:03 larsoner

Hey, @cbrnr @drammock @larsoner I have updated everything based on your suggestions. Can we merge this request?

hasrat17 avatar Mar 19 '24 04:03 hasrat17

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

welcome[bot] avatar Mar 19 '24 16:03 welcome[bot]