search

Python-Markdown / markdown

A Python implementation of John Gruber’s Markdown with Extension support.
https://python-markdown.github.io/
BSD 3-Clause "New" or "Revised" License
3.79k stars 863 forks source link

modernized syntax allowed for py37+ #1304

Closed venthur closed 1 year ago

venthur commented 1 year ago

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

this is related to #760

waylan commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

venthur commented 1 year ago

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 commented 1 year ago

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

waylan commented 1 year ago

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