Closed Bug 1644863 Opened 4 months ago Closed 4 months ago

Expand whitespace / line-endings linter to JS, CSS, HTML and l10n files

Categories

(Firefox Build System :: Lint and Formatting, task)

task

Tracking

(firefox79 fixed)

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(4 files)

We have a linter that complains about trailing whitespace and windows line endings, https://searchfox.org/mozilla-central/source/tools/lint/file-whitespace/__init__.py . It's invoked by this configuration file: https://searchfox.org/mozilla-central/source/tools/lint/file-whitespace.yml and for changes in CI based on https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/taskcluster/ci/source-test/mozlint.yml#331-349 . This only lints C++, rust and python files.

It would be nice to do the same for:

*.jsm
*.js
*.css
*.xhtml
*.html

*.ftl
*.properties
*.dtd

Prettier should already be managing jsm and js files: https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/.prettierrc#2

I know it will do some formatting for xhtml/html, though we don't have that enabled at the moment.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #9155799 - Attachment description: Bug 1644863 - fix trailing whitespace in cross-tree tests, r?emilio! → Bug 1644863 - fix trailing whitespace in layout CSS and cross-tree tests, r?emilio!

@Pike
Do you see any problem in having trailing spaces trimmed in localization files? Thinking mostly in terms of cross-channel content when this lands.

Fluent has a specific way to deal with trailing whitespaces, and we enforced all trailing spaces to be converted to \u0020 in .properties, so I don't expect issues there. I've also checked D79203 and it looks good, only trailing spaces and wrong line endings (sigh).

Flags: needinfo?(l10n)

(In reply to Francesco Lodolo [:flod] from comment #6)

@Pike
Do you see any problem in having trailing spaces trimmed in localization files? Thinking mostly in terms of cross-channel content when this lands.

Fluent has a specific way to deal with trailing whitespaces, and we enforced all trailing spaces to be converted to \u0020 in .properties, so I don't expect issues there. I've also checked D79203 and it looks good, only trailing spaces and wrong line endings (sigh).

I should maybe note explicitly that I made an exception for https://searchfox.org/mozilla-central/rev/4bb2401ecbfce89af06fb2b4d0ea3557682bd8ff/toolkit/locales/en-US/chrome/global/mozilla.dtd#9-12 . I think we don't need the trailing space, because there is a newline and at least in HTML that would just get shown as a single space anyway, but it was like 1am when I was writing the patch and I just didn't want to worry about it - I assume we'll get rid of it when converting that stuff to FTL.

I need to have a deeper look, unless flod beats me to it.

I'm mostly concerned about the changes to comments leading to duplicated comments in the merged content.

One thing to do would be to pull this revision and run cross-channel against it, and see what actually happens. Happy to look into that when I'm back from PTO.

For now, I left myself as a blocking reviewer on the l10n patch.

Flags: needinfo?(l10n)

(In reply to Axel Hecht [:Pike] from comment #8)

One thing to do would be to pull this revision and run cross-channel against it, and see what actually happens. Happy to look into that when I'm back from PTO.

Sounds good to me, and I have no idea how to test things locally without breaking things :-)

In the meantime, this landed, and we might be lucky
https://hg.mozilla.org/users/axel_mozilla.com/gecko-strings-quarantine/rev/4704a134c94204abe54fb86ff209d33c634ff438

Comments were updated, and no duplication. No clue what's the criteria that causes comments to be duplicated or not, since I've seen both.

Attachment #9155799 - Attachment description: Bug 1644863 - fix trailing whitespace in layout CSS and cross-tree tests, r?emilio! → Bug 1644863 - fix trailing whitespace in cross-tree tests, r?emilio!
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2a9ac536f873
fix trailing whitespace in browser/toolkit/devtools/mobile frontend code files, r=mossop,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/1b9de9e2fc19
fix trailing whitespace in cross-tree tests, r=emilio,marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/2b4dde8c9c1e
fix trailing whitespace and windows line endings in locale files, r=fluent-reviewers,flod,Pike
https://hg.mozilla.org/integration/autoland/rev/987355bf1f1a
include CSS, HTML and localization (ftl, properties, dtd) files in the whitespace linter, r=sylvestre
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.