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

Move non_send_fields_in_send_ty to suspicious #7874

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Oct 24, 2021

Stabilize the non_send_fields_in_send_ty lint and update the lint for the allowed_scripts configuration.


closes: #7756

changelog: Move non_send_fields_in_send_ty to suspicious

@rust-highfive
Copy link

r? @llogiq

(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 Oct 24, 2021
@xFrednet xFrednet force-pushed the 7756-move-lint branch 2 times, most recently from c4d2de1 to 28a5b68 Compare October 24, 2021 21:16
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Just one question, otherwise this looks good to me.

///
/// The list of unicode scripts allowed to be used in the scope.
(allowed_scripts: Vec<String> = vec!["Latin".to_string()]),
(allowed_scripts: Vec<String> = ["Latin"].iter().map(ToString::to_string).collect()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the reason for using an iterator here. What was wrong with using vec![_] before?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remembered an old implementation, that would only display the content inside the square brackets to make the documentation a bit cleaner. Now it actually uses format!("{:?}", default_value) which doesn't care about the formatting. The cases are therefore equivalent 😅

I would still suggest to switch it for consistency, as all other string vectors do the same. However, I'm also happy to revert this again 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps just add a comment that this test case also covers the distinction between vec! and some_iter.collect() so future reviewers don't have to wonder.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't really have any test case for the metadata collecting 😅. However, I can update the "Add configuration" documentation to say that the default value will be formatted with format!("{:?}", default)

Also updated one configuration for nicer formatting
@llogiq
Copy link
Contributor

llogiq commented Oct 25, 2021

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Oct 25, 2021

📌 Commit 1ad04f4 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Oct 25, 2021

⌛ Testing commit 1ad04f4 with merge 4f46f42...

@bors
Copy link
Contributor

bors commented Oct 25, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 4f46f42 to master...

@bors bors merged commit 4f46f42 into rust-lang:master Oct 25, 2021
@xFrednet xFrednet deleted the 7756-move-lint branch November 11, 2021 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move non_send_fields_in_send_ty to suspicious in Rust 1.58
4 participants