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

Use the same feature name validation rule from Cargo #7500

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Nov 10, 2023

Split from #7379

  • This PR allowed . in the feature name. I added some unit tests and integration tests for it.
  • This PR also updated the feature name validation rules from Cargo.

The summary:

category Cargo crates.io before change crates.io after change
Normal feature name 1. can only start with most letters, _, or 0 to 9
2. can only contain numbers, +, -, _, ., or most letters
can only contain numbers, +, -, _, or most letters 1. can only start with most letters, _, or 0 to 9
2. can only contain numbers, +, -, _, ., or most letters

Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check

Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

🔢 Self-check again

@Rustin170506
Copy link
Member Author

This PR is ready for review.
Could you please take a look? Thanks! 💚 💙 💜 💛 ❤️

@Rustin170506 Rustin170506 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Nov 13, 2023
@Turbo87
Copy link
Member

Turbo87 commented Nov 14, 2023

unfortunately this PR still mixes the refactoring of the error messages with the change of the validation rules. reviewing commit-by-commit does not help because 2765157 does both at the same time too. this makes it significantly harder to see which change belongs to the error message refactoring and which change belongs to the validation rules change.

@Rustin170506
Copy link
Member Author

unfortunately this PR still mixes the refactoring of the error messages with the change of the validation rules.

I have to change the error because now we support the Unicode character in the feature name. I don't see any benefits to keeping that unclear error message.

commit-by-commit does not help because 2765157 does both at the same time too. this makes it significantly harder to see which change belongs to the error message refactoring and which change belongs to the validation rules change.

It's just two functions with about 50 line changes. And I just copied it from the Cargo repo. I think it's a waste of time, so I'm closing my PR.

@Turbo87
Copy link
Member

Turbo87 commented Nov 14, 2023

I have to change the error because now we support the Unicode character in the feature name. I don't see any benefits to keeping that unclear error message.

I'm not saying "don't do it". I'm just saying do one after the other. If adding support for the unicode character requires us to refactor our error handling then refactor the error handling first, commit that, and then implement the behavior change on top. This makes it significantly easier to review and less likely to introduce unintentional bugs.

@Rustin170506
Copy link
Member Author

I'm not saying "don't do it". I'm just saying do one after the other. If adding support for the unicode character requires us to refactor our error handling then refactor the error handling first, commit that, and then implement the behavior change on top. This makes it significantly easier to review and less likely to introduce unintentional bugs.

I can get your point. If this is a new feature or a really big change to the rules. I think it makes sense. But you asking me to split 30 lines of code which I copied from Cargo into two commits. I don't think it makes sense. It is wasting our time.

I have spent a bunch of time making sure I can help the reviewing process. And I keep receiving negative feedback from my pull request. You just keep asking to change every line code in your approach. Nobody can 100% follow your code style to write code. It's just 30 lines of code. And I didn't break any tests and also added a lot of tests to check it. so I am so frustrated to move forward with this PR. So I decided to close it.

@Rustin170506
Copy link
Member Author

Thank you for taking the time to review it.

@Rustin170506 Rustin170506 reopened this Nov 14, 2023
@Rustin170506
Copy link
Member Author

Let me try it agian:(

@Rustin170506
Copy link
Member Author

@Turbo87 2ae50e5 This commit is only for the validation rules changes. I hope this helps.

@bors
Copy link
Contributor

bors commented Nov 14, 2023

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

invalid_feature and invalid_feature_name both for test invalid feature name.
So we can use the same prefix for them.

Signed-off-by: hi-rustin <[email protected]>
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

Rebased to fix the lockfile merge conflict. I have a couple more suggestions on how to improve the code, but those can be added afterwards as well.

@Turbo87 Turbo87 merged commit 9fd9306 into rust-lang:main Nov 15, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants