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

rule: add union_now_doc_hidden #679

Merged
merged 4 commits into from
Mar 2, 2024
Merged

Conversation

vasucp1207
Copy link
Contributor

@vasucp1207 vasucp1207 commented Mar 1, 2024

Part of #578

  • More tests

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

So far this looks good! Let me know if you get stuck on anything and if you'd like me to take a closer look.

pub union MyUnion {
f1: u32,
f2: f32,
}
Copy link
Owner

Choose a reason for hiding this comment

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

nitpick: could you ensure all files have trailing newline characters? You should be able to adjust your editor to add them automatically when you save.

Copy link
Owner

Choose a reason for hiding this comment

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

I've never seen cargo.toml as the filename for this, I've only ever seen Cargo.toml (note the capitalization difference).

I'm curious how you generated this file — did you use the scripts for creating new lints and test files? Are you using a filesystem that lowercases everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason Cargo.toml is not generated after running the script but all other files do. My bad I named it wrongly.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh interesting, that sounds like a bug in the script that we should fix. Which script did you have this experience with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running ./scripts/regenerate_test_rustdocs.sh also gives the error,
cargo-semver-checks/scripts/regenerate_test_rustdocs.sh: line 23: shopt: globstar: invalid shell option name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After commenting out line no 23 it works fine.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah can you check which shell and which version of that shell you're using? globstar was introduced in bash 4 (released 15 years ago!) but some systems still only have bash 3 by default =/

Our scripts should probably check the shell version and exit with an error if it's too old. If you're open to writing and testing that kind of PR on your machine, I'd be thrilled to merge it — I'm not able to because all my computers are updated to use bash 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting, that sounds like a bug in the script that we should fix. Which script did you have this experience with?

make_new_lint.sh

Copy link
Owner

Choose a reason for hiding this comment

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

Opened #680 for this, feel free to tackle it if you find it interesting! Our tooling gets better one small improvement at a time — it really adds up over the course of weeks and months.

@vasucp1207 vasucp1207 marked this pull request as ready for review March 2, 2024 00:02
@pksunkara
Copy link
Contributor

Looks like this lint can be unified too with enum and struct. Not a blocker for this PR though.

@obi1kenobi
Copy link
Owner

Looks like this lint can be unified too with enum and struct. Not a blocker for this PR though.

Yeah, lots of lints can be unified like that. At this stage that's predominantly a cosmetic improvement, though, so having the lint in any form is better than trying to squeeze everything into as few lints as possible.

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations labels Mar 2, 2024
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Nicely done! 🚀

@obi1kenobi obi1kenobi merged commit 2c334a7 into obi1kenobi:main Mar 2, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants