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.

Sign up for GitHub

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

Jump to bottom

Move code generated by update_lints to includes #7673

Merged
merged 6 commits into from Sep 30, 2021
Merged

Move code generated by update_lints to includes #7673

merged 6 commits into from Sep 30, 2021

Conversation

ghost
Copy link

@ghost ghost commented Sep 15, 2021

Move code generated by update_lints to includes

changelog: none

@rust-highfive
Copy link

r? @giraffate

(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 Sep 15, 2021
@ghost
Copy link
Author

ghost commented Sep 15, 2021 •
edited by ghost
Loading

The idea with this pull request is to move code generated out of lib.rs and into includes.

The benefit is that moves a lot of clutter out of lib.rs. It's also easier to do the generation as a whole file can be replaced instead of a region and these files aren't formatted by rustfmt.

Some things still be done are:

  • Fix cargo dev new_lint
  • Agree on what the include filenames should be
  • Agree on the format of the generated code (e.g. comment at the top, end in new line etc.)
  • Refactor the update_lints code

This is a big change and it might be a bad idea. Feel free to close.

I'll finish this off if it gets a thumbs up.

flip1995
flip1995 reviewed Sep 15, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I like this change. This will also make syncing with rustc easier, since conflicts in those auto-generated files can just be ignored and lib.rs won't have as much churn.

Agree on the format of the generated code (e.g. comment at the top, end in new line etc.)

I would add a newline at the end of the file and keep the format consistent. Some files have indentation, some don't. I would format those files as close as possible to what rustfmt would do. (see comments below)

Before you continue working on this, I'd wait for more feedback from @rust-lang/clippy though

clippy_lints/src/lib.deprecated.rs Show resolved Hide resolved
clippy_lints/src/lib.register_all.rs Outdated Show resolved Hide resolved
clippy_lints/src/lib.register_lints.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Sep 15, 2021 •
edited
Loading

I like the idea, +1 from me.

If we continue this, could you please make sure to also include the automatically generated files in our pre-commit hook? 🙃

rust-clippy/util/etc/pre-commit.sh

Lines 7 to 8 in 2316f4d

cargo dev update_lints
git add clippy_lints/src/lib.rs

@giraffate
Copy link
Contributor

Sounds like a good idea, I like it.

@bors
Copy link
Contributor

bors commented Sep 17, 2021

☔ The latest upstream changes (presumably #7653) made this pull request unmergeable. Please resolve the merge conflicts.

@ghost ghost marked this pull request as ready for review September 18, 2021 04:58
@bors
Copy link
Contributor

bors commented Sep 22, 2021

☔ The latest upstream changes (presumably #7693) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Sep 24, 2021

☔ The latest upstream changes (presumably #7715) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member

Do we want to move this PR forward once it was rebased? There seems to be no objections so far 🙃

@giraffate
Copy link
Contributor

Sorry for the late reviewing. I've been a bit busy. Please give me a few more days.

flip1995
flip1995 approved these changes Sep 27, 2021
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Generated files LGTM. I didn't review the clippy_dev changes.

There were no objections to this in the last meeting, so once @giraffate is done with the review, this can be merged.

@bors
Copy link
Contributor

bors commented Sep 27, 2021

☔ The latest upstream changes (presumably #7692) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Sep 28, 2021

☔ The latest upstream changes (presumably #7608) made this pull request unmergeable. Please resolve the merge conflicts.

Michael Wright added 5 commits September 29, 2021 05:43
Move code generated by update_lints to includes
50ea370
Fix lint register code format
e6747df 
Also change the generation functions to return `String` instead of
`Vec<String>`. This makes sense now as the updates aren't line oriented
anymore.
Move update_lints specific code out of lib
3f804ca
Add test module for update_lints
20abbd9
Add includes to pre-commit hook
6035e05
giraffate
giraffate approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, again.

Over all looks good, Thanks! I made one minor comment.

clippy_dev/src/update_lints.rs Outdated Show resolved Hide resolved
@giraffate
Fix comment in update_lints
debb1f0 
Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com>
@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Sep 30, 2021

📌 Commit debb1f0 has been approved by giraffate

@bors
Copy link
Contributor

bors commented Sep 30, 2021

⌛ Testing commit debb1f0 with merge 984d466...

@bors
Copy link
Contributor

bors commented Sep 30, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 984d466 to master...

@bors bors merged commit 984d466 into rust-lang:master Sep 30, 2021
This was referenced Sep 30, 2021
Merged
Merged
This was referenced Sep 30, 2021
Closed
Merged
Merged
Merged
@ghost ghost deleted the include-generated-code branch September 30, 2021 05:45
@Manishearth
Copy link
Member

Ooh, this has made rustfmt stop working for the entire codebase since the modules are include!d and rustfmt doesn't read that.

@Manishearth Manishearth mentioned this pull request Oct 6, 2021
Merged
@Manishearth
Copy link
Member

Trying to fix that in #7773

bors added a commit that referenced this pull request Oct 6, 2021
@bors
Auto merge of #7773 - Manishearth:update-lints-mods, r=flip1995
b505e94 
Move module declarations back into lib.rs

With #7673 we moved a lot of things from lib.rs to lib.foo.rs. Unfortunately, rustfmt doesn't seem to work when module declarations are included via `include!` (and trying the `mod foo; use foo::*;` trick doesn't seem to work much either in our specific case).

With this PR we continue generating everything in subfiles except for module declarations, which are now generated within lib.rs.
bors added a commit that referenced this pull request Oct 6, 2021
@bors
Auto merge of #7773 - Manishearth:update-lints-mods, r=flip1995
871b8b5 
Move module declarations back into lib.rs

With #7673 we moved a lot of things from lib.rs to lib.foo.rs. Unfortunately, rustfmt doesn't seem to work when module declarations are included via `include!` (and trying the `mod foo; use foo::*;` trick doesn't seem to work much either in our specific case).

With this PR we continue generating everything in subfiles except for module declarations, which are now generated within lib.rs.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@giraffate giraffate

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rust-highfive @xFrednet @giraffate @bors @Manishearth @flip1995