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

Update features heuristic to include more unstable and private feature name patterns #463

Merged
merged 8 commits into from
May 31, 2023

Conversation

era
Copy link
Contributor

@era era commented May 31, 2023

Addresses #461, by adding _, unstable- and unstable_ prefixes to ignore.

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.

This looks good, just a couple of small nitpicks.

I've found that reading one's own code in the GitHub PR review interface makes it more likely to spot these kinds of little typos. We all get used to looking at our code changes in our local editor and the typos become invisible, so the change of scenery helps make them stand out.

I'd highly recommend trying it, since it would both increase your velocity and reduce the number of back-and-forth code review cycles before merging. I think you could have noticed all the things I pointed out in this review by yourself with one self-review like that.

src/rustdoc_gen.rs Outdated Show resolved Hide resolved
test_crates/features_simple/old/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 149 to 153
let prefix_ignored_by_default = vec![
String::from("_"),
String::from("unstable-"),
String::from("unstable_"),
];
Copy link
Owner

Choose a reason for hiding this comment

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

Out of curiosity, do we need the String::from() here and above? Would this code work if these were static &str instead?

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, it would! I'm cleaning it to be &str instead.

Comment on lines 161 to 162
.map(|p| feature_name.starts_with(p))
.any(|f| f)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is just .any(|p| feature_name.starts_with(p)) right?

era and others added 5 commits May 31, 2023 17:29
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
@era
Copy link
Contributor Author

era commented May 31, 2023

Thank you for the review, I properly setup my editor, so I should not have that type of problem anymore :).

Thank you again for all the tips,

@obi1kenobi obi1kenobi merged commit 817b1ff into obi1kenobi:main May 31, 2023
@obi1kenobi
Copy link
Owner

@era awesome work! Are you happy staying on the CLI side for now or would you like to try your hand at writing some lints as well? It's all the same to me, so it's your call :)

The docs for writing lints are here and I think I linked to a few easy-ish lints to start with in a prior chat.

@era
Copy link
Contributor Author

era commented May 31, 2023

Ah, thank you for pointing me to the lint docs, I think I will try to do one lint to see how it goes! Thank you again for all mentoring around the project!!

@obi1kenobi
Copy link
Owner

No worries, thanks for all the awesome contributions — it's my pleasure to help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants