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

modernized syntax allowed for py37+ #1304

Closed
wants to merge 1 commit into from

Conversation

venthur
Copy link
Contributor

@venthur venthur commented Nov 15, 2022

This pr modernizes the syntax for things that are allowed for pyhton3.7+. It is mostly

  • ''.format -> f''
  • set() -> {}
  • removal of explicit utf8 encoding of files

this is related to #760

This pr modernizes the syntax for things that are allowed for
pyhton3.7+. It is mostly

* ''.format -> f''
* set() -> {}
* removal of explicit utf8 encoding of files
@waylan
Copy link
Member

waylan commented Nov 15, 2022

Please note that the Contributing Guide states the following:

Legacy code which does not follow the guidelines should only be updated if and when other changes (bug fix, feature addition, etc.) are being made to that section of code.

That said, .format -> f' could be classified as a performance improvement, especially if we were provided some numbers to back it up. However, that should be a separate PR by itself. And removal of explicit utf8 encoding of files could be considered code cleanup I suppose (again as a separate PR. However, set() -> {} is clearly a style thing and the sort of change our Contributing Guide discourages.

@venthur
Copy link
Contributor Author

venthur commented Nov 15, 2022

Alright, I thought it might help to cleanup the code a bit, but feel free to reject the PR if it is not needed.

@waylan
Copy link
Member

waylan commented Nov 15, 2022

As I said, if you want to do the work to show that the .format -> f' change provides us with a clear performance improvement, then we would be happy to accept the work.

However, I don't actually see any changes which remove explicit utf8 encoding of files. So if you want to reclassify the PR to be about the first issue you can. Just remove the set changes.

@waylan waylan added the requires-changes Awaiting updates after a review. label Nov 15, 2022
@venthur
Copy link
Contributor Author

venthur commented Nov 15, 2022

As I said, if you want to do the work to show that the .format -> f' change provides us with a clear performance improvement, then we would be happy to accept the work.

No not really, actually. If you don't like the proposal you can just go ahead and close it. No hard feelings.

However, I don't actually see any changes which remove explicit utf8 encoding of files. So if you want to reclassify the PR to be about the first issue you can. Just remove the set changes.

It is at the very bottom of the diff:

https://github.com/Python-Markdown/markdown/pull/1304/files#diff-57787454b56d61377fe6ec8e971df1e1e1ec9d6f1f0dd4a13209bf5bfb55633bL1

The idea of the PR was to make the code more readable, I was not aware of that rather strict guideline not to touch code unless it fixes something else. Sorry for the noise.

@akx
Copy link
Contributor

akx commented Dec 17, 2022

Seems to be a revisit of my #1215, fwiw.

@waylan
Copy link
Member

waylan commented Dec 19, 2022

I'm closing this. If an update to provided which addresses the concerns raised, we can reopen.

@waylan waylan closed this Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-changes Awaiting updates after a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants