Skip to content

Conversation

mondeja
Copy link
Contributor

@mondeja mondeja commented Oct 11, 2021

  • Replaced some .format calls with f-strings.
  • Removed 'r' mode argument for open function as it is uneeded, but maybe maintainers want to keep it explicit?
  • Removed __future__ import and coding: utf-8 comment from a file.

@oprypin
Copy link
Contributor

oprypin commented Oct 11, 2021

Note that in #2301 I already went over all the occurrences of format in the codebase and changed to f-strings only in the cases where I thought it would be an improvement. Most of these I think are harder to read with this change.

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

To give a clearer comment...

Now I will point out the particular places where subjectively I think the code "after" is harder to understand than the code "before".
Sorry that I'm not giving the reasoning.

For an easy way forward, maybe you could just undo those parts. The rest is all fine.

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

Further discussion is also possible.

Certainly there's a lot of merit to setting a clear guideline once and for all.

Some examples of guidelines:

  1. Always use f-strings, not .format
    (what seems like you're trying).
  2. Use .format if one of the items isn't a plain variable or member access
    (seems like the current situation, I don't mind it).
  3. If one of the items isn't a plain variable or member access, first assign it to a variable and then always use f-strings anyway
    (I could be open to this, but I think it can also be messy).

@oprypin
Copy link
Contributor

oprypin commented Oct 12, 2021

The last commit - I don't think it's good to clump it together with this pull request. Please drop it.

Also not sure about "Rewrite Nav config type validation error message" - ideally would be separate

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

Thanks. And separately thanks for your patience :)

@ultrabug
Copy link
Member

Thanks a lot!

@ultrabug
Copy link
Member

I'm stupid and merged in the wrong order @mondeja , I apologize for the conflict here...

@mondeja
Copy link
Contributor Author

mondeja commented Oct 13, 2021

No problem, it's fixed now 👍🏼

@oprypin oprypin merged commit 1da44ce into mkdocs:master Oct 24, 2021
@mondeja mondeja deleted the py36+ branch October 24, 2021 18:06
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.

3 participants