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

tests: ruff: enforce UP032 (f-string) rule #5725

Closed
wants to merge 3 commits into from

Conversation

KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Jun 27, 2024

TODO:

  • Either fix pango markup checker needs to support f-strings pocketlint#51
  • DONE: Or revert the f-string changes on the set_markup
  • Or disable pylint markup checks on the affected lines
  • Check if this somehow affecting the translations (I am not able to test that, the generated boot.iso does not have po files inside I think)

@KKoukiou KKoukiou temporarily deployed to gh-cockpituous June 27, 2024 12:59 — with GitHub Actions Inactive
@github-actions github-actions bot added the f41 label Jun 27, 2024
Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Obviously these were autofixed:

ruff check --fix --select UP032 .

@KKoukiou KKoukiou marked this pull request as draft June 27, 2024 13:31
KKoukiou added 3 commits June 27, 2024 15:52
These were uncovered with the f-string porting:
Let's autfix these as well with: ruff check --fix --select RUF010 .
@KKoukiou KKoukiou temporarily deployed to gh-cockpituous June 27, 2024 13:59 — with GitHub Actions Inactive
@KKoukiou KKoukiou marked this pull request as ready for review June 27, 2024 14:00
Copy link
Contributor Author

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

@vojtechtrefny do you know if there is any reason to not do this?

@KKoukiou
Copy link
Contributor Author

/build-image

@KKoukiou
Copy link
Contributor Author

/kickstart-test --testtype smoke

Copy link

Images built based on commit 8417ef2:

  • boot.iso: success

Download the images from the bottom of the job status page.

@KKoukiou KKoukiou requested a review from jkonecny12 June 28, 2024 09:40
@jkonecny12
Copy link
Member

This change is against Code conventions set by the team: https://anaconda-installer.readthedocs.io/en/latest/contributing.html#code-conventions

Format strings with [.format()](https://docs.python.org/3/library/stdtypes.html#str.format) instead of % (https://pyformat.info/)
Exception: Use % formatting in logging functions and pass the % as arguments. See [logging format interpolation](https://stackoverflow.com/questions/34619790/pylint-message-logging-format-interpolation) for the reasons.

Reason we went with this was that we did not wanted to mixup f-strings with format and format is not able to cover all the cases. We can definitely reconsider this solution again but I'm marking this as blocked as it's against the current code conventions we have set.

@jkonecny12 jkonecny12 added the blocked Don't merge this pull request! label Jul 1, 2024
@KKoukiou
Copy link
Contributor Author

KKoukiou commented Jul 1, 2024

This change is against Code conventions set by the team: https://anaconda-installer.readthedocs.io/en/latest/contributing.html#code-conventions

Format strings with [.format()](https://docs.python.org/3/library/stdtypes.html#str.format) instead of % (https://pyformat.info/)
Exception: Use % formatting in logging functions and pass the % as arguments. See [logging format interpolation](https://stackoverflow.com/questions/34619790/pylint-message-logging-format-interpolation) for the reasons.

Reason we went with this was that we did not wanted to mixup f-strings with format and format is not able to cover all the cases. We can definitely reconsider this solution again but I'm marking this as blocked as it's against the current code conventions we have set.

As I mentioned in the past as well, this file is read by noone, it's not enforced actively and only the ultra old team members might be able to refer to it like you did now.
The way to go forward it to enfore rules through linters (pylint, ruff etc) and keep the configuration files for these as the single source of truth.

So, on the decision for fstrings - I actually think it's the way to go, the only place it does not apply is the pango templates, because of the custom pocketlint checks, which are not covering fstrings (see commit message)

Otherwise, we should really be able to delete the conventions file.

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Jul 1, 2024

@jkonecny12 which cases does fstrings does not cover?

@jkonecny12
Copy link
Member

Ahh, my memory is failing here. IIRC f-strings can't be modified before you will mark them as f which is causing troubles for example with localization strings.

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Jul 1, 2024

Ahh, my memory is failing here. IIRC f-strings can't be modified before you will mark them as f which is causing troubles for example with localization strings.

Can you elaborate here a bit more?

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Jul 2, 2024

@KKoukiou KKoukiou closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Don't merge this pull request! f41
Development

Successfully merging this pull request may close these issues.

2 participants