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

[ui, compliance] Remove the newline after .hbs copyright headers #16861

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

philrenaud
Copy link
Contributor

We were seeing test failures in main related to log-content equivalence tests, as well as some flakey percy vertical spacing issues, that seem to point to the newline after copyright headers. This PR attempts to remove that newline across all our .hbs files in the ui, to see if those problems are resolved.

image
image

@github-actions
Copy link

github-actions bot commented Apr 12, 2023

Ember Asset Size action

As of e1ef6df

Files that got Smaller 🎉:

File raw gzip
nomad-ui.js -2.33 kB -42 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Apr 12, 2023

Ember Test Audit comparison

main e1ef6df change
passes 1482 1489 +7
failures 7 0 -7
flaky 0 0 0
duration 000ms 11m 01s 706ms +11m 01s 706ms

@lgfa29
Copy link
Contributor

lgfa29 commented Apr 12, 2023

Have you tested with ~}} for white-space control? I'm afraid that without fixing the bot we will keep running into this problem and having to manually fix new files.

@tgross
Copy link
Member

tgross commented Apr 12, 2023

I'm afraid that without fixing the bot we will keep running into this problem and having to manually fix new files.

It looks like we can run copywrite locally to see if it'll fail these files again.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM but worth verifying it won't get undone by the bot again

@philrenaud
Copy link
Contributor Author

~}}

@lgfa29 very pleased to report that the ~}} style syntax works exactly the same as removing the newline altogether!

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

Great! Looking at copywrite source code it seems like it only checks for the license information, not the exact comment structure, so it won't try to change these files.

But I think it would be worth a PR to update .hbs headers to look like this and avoid the unnecessary line break. This will ensure new files are correct from the beginning.

I can take care of opening the PR, unless you really want to 😄

@philrenaud philrenaud merged commit f74623a into main Apr 14, 2023
@philrenaud philrenaud deleted the ui-copyright-headers-in-hbs-no-newline branch April 14, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants