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

tidy: check fluent files for style #100671

Merged
merged 2 commits into from
Aug 22, 2022
Merged

Conversation

Xiretza
Copy link
Contributor

@Xiretza Xiretza commented Aug 17, 2022

Inspired by #100651 (comment)

There were a lot of line length violations, so I've excepted that lint - I'm not sure if fluent files can be formatted to avoid long lines at all.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 17, 2022
@rust-highfive
Copy link
Collaborator

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2022
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for the patch!

compiler/rustc_error_messages/locales/en-US/borrowck.ftl Outdated Show resolved Hide resolved
@davidtwco davidtwco added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Aug 17, 2022
@davidtwco
Copy link
Member

r? @davidtwco

@rust-highfive rust-highfive assigned davidtwco and unassigned jyn514 Aug 17, 2022
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Great! Will r=me once CI passes.

@davidtwco
Copy link
Member

cc @Mark-Simulacrum for the tidy change

@davidtwco
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 17, 2022

📌 Commit fd05fa0 has been approved by davidtwco

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2022
@est31
Copy link
Member

est31 commented Aug 17, 2022

I'm not sure this PR is a good idea, at least for non-english fluent files, because those will be edited using a tool (at least according to what @Manishearth said), and that tool might not create ftl files that conform with tidy checks. Even with the submodules/subtrees that rustc uses, a lot of them are exempt from tidy, see the filter_dirs function in lib.rs, partially because the CI of those projects don't enforce tidy either, so it might cause issues with submodule upgrades. Note that my complaint only applies to the non-english fluent files, as it is my understanding that the english ftl files are kinda special as they will be maintained outside of the tool (or idk).

@davidtwco
Copy link
Member

I'm not sure this PR is a good idea, at least for non-english fluent files, because those will be edited using a tool (at least according to what @Manishearth said), and that tool might not create ftl files that conform with tidy checks. Even with the submodules/subtrees that rustc uses, a lot of them are exempt from tidy, see the filter_dirs function in lib.rs, partially because the CI of those projects don't enforce tidy either, so it might cause issues with submodule upgrades. Note that my complaint only applies to the non-english fluent files, as it is my understanding that the english ftl files are kinda special as they will be maintained outside of the tool (or idk).

English Fluent will be maintained manually like it is now. We can add whatever exceptions we need for submodules or translations or however we end up handling that, so I think this should be okay.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 17, 2022
…twco

tidy: check fluent files for style

Inspired by rust-lang#100651 (comment)

There were a lot of line length violations, so I've excepted that lint - I'm not sure if fluent files can be formatted to avoid long lines at all.
@matthiaskrgr
Copy link
Member

@bors r- files are actually causing errors
#100701 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2022
@Xiretza
Copy link
Contributor Author

Xiretza commented Aug 18, 2022

My plan is to wait until #100651 lands, then rebase this (fixing the new formatting problems) and hope it doesn't race another PR that adds whitespace problems. Sound sensible?

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank

@Xiretza
Copy link
Contributor Author

Xiretza commented Aug 18, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2022
@davidtwco
Copy link
Member

I'll approve this but not for inclusion in rollups just in case it gets mixed in with the other translation PRs that are happening and makes things fail.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 18, 2022

📌 Commit 2954571 has been approved by davidtwco

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2022
@bors
Copy link
Contributor

bors commented Aug 22, 2022

⌛ Testing commit 2954571 with merge d0ea1d7...

@bors
Copy link
Contributor

bors commented Aug 22, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing d0ea1d7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2022
@bors bors merged commit d0ea1d7 into rust-lang:master Aug 22, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 22, 2022
@Xiretza Xiretza deleted the tidy-fluent-files branch August 22, 2022 06:41
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d0ea1d7): comparison url.

Instruction count

  • Primary benchmarks: ✅ relevant improvement found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% -1.0% 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.0% -1.0% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: ❌ relevant regressions found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions ❌
(primary)
3.2% 4.0% 2
Regressions ❌
(secondary)
2.1% 3.5% 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% -2.9% 1
All ❌✅ (primary) 3.2% 4.0% 2

Cycles

Results
  • Primary benchmarks: ✅ relevant improvement found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.5% -3.5% 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.5% -3.5% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 23, 2022
… r=davidtwco

fluent: mandate slug names to be prefixed by crate name

This is currently only convention, but not actively checked for.

Additionally, improve error messages to highlight the path of the offending fluent file rather than the identifier preceding it.

This will conflict with rust-lang#100671, so I'll leave it as draft until that's merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants