-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-127111: Apply prettier formatter to Emscripten web example #127551
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
Conversation
Fixed one spot where there was invalid html (script tag closed twice). Other than that, this gives consistent usage of semicolons, quotes, indentation, etc. Separated out into its own commit for ease of review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing format change PRs is always hard, but AFAICT this is all straightforward stuff. One question inline about some metadata that probably needs feedback from @katharosada, and we should be good to go.
<meta charset="UTF-8" /> | ||
<meta http-equiv="X-UA-Compatible" content="IE=edge" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
<meta name="author" content="Katie Bell" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to deny @katharosada credit here - but should this author
be preserved? My concern is that given this is example code, the likelihood of the <head>
block being copy-pastad into other projects seems high. Looking at other code in the Python codebase, the only places a specific name is called out is in a copyright assertion; and there, it's always in a comment, rather than in something that is "externally visible" as it would be here.
Unless there's a credit or copyright assertion reason for preserving this, I think we'd be better served removing the author meta tag - but I don't want to do that without @katharosada's informed consent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it might make sense to remove this author tag. But shouldn't this be done in a separate PR? I'd like to keep all the automatic noisy formatting changes by themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - I can live with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure that author tag was added automatically by my IDE or some plugin when I created the HTML file, I didn't even notice it.
Feel free to remove, no argument here!
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
<meta charset="UTF-8" /> | ||
<meta http-equiv="X-UA-Compatible" content="IE=edge" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
<meta name="author" content="Katie Bell" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - I can live with that.
…ython#127551) Cleaned up formatting (and a stray closing tag) of the web example HTML and JS.
Fixed one spot where there was invalid html (script tag closed twice). Other than that, this gives consistent usage of semicolons, quotes, indentation, etc. Separated out into its own commit for ease of review.