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

Add pub mod option for UnlinkedFile #8345

Merged
merged 1 commit into from May 18, 2021

Conversation

yue4u
Copy link
Contributor

@yue4u yue4u commented Apr 5, 2021

close #8228

This is a draft that changes Diagnostic to contain multiple fixes. Pre analysis is in #8228 (comment) Because this solution is straightforward so I decided to type it out for discussion.

Currently the check_fix is not able to test the situation when multiple fixes available. Also because Insert 'mod x;' and Insert 'pub mod x;' are so similar, I don't know how to test them correctly and want some suggestions.. I added
check_fixes to allow checking mutiple possible fixes.

In additional, instead of append after possible existing mod y, I think it's possible to Insert pub mod x; after pub mod y. Should I implement this too?

@yue4u yue4u marked this pull request as ready for review April 5, 2021 21:26
@yue4u yue4u force-pushed the add-pub-mod-option-for-unlinked-files branch 2 times, most recently from 1cfe45f to a74e1fd Compare April 10, 2021 01:48
pub(crate) trait DiagnosticWithFix: Diagnostic {
fn fix(&self, sema: &Semantics<RootDatabase>) -> Option<Fix>;
pub(crate) trait DiagnosticWithFixes: Diagnostic {
fn fixes(&self, sema: &Semantics<RootDatabase>) -> Option<Vec<Fix>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an important invariant that the vector must be non-empty if this returns Some? If not I think this should just return Vec<Fix> without the option.

Copy link
Contributor Author

@yue4u yue4u Apr 14, 2021

Choose a reason for hiding this comment

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

As I explained in #8345 (comment) as a second option, I think Vec<Fix>(became Vec<Assist> after rebasing) is better too. However functions inside fix currently make extensive use of Try trait and turning them to return Vec<Fix> without Option doesn't seem easy to do.

e.g:
https://github.com/rust-analyzer/rust-analyzer/blob/10a243ea55565a0dd1de52f8f802c3e3a7bfef54/crates/ide/src/diagnostics/fixes.rs#L162-L176

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I think it also makes sense to change it to return something like Option<HashMap<AssistId,Assist>> which would possibly make it easier to test a specific Assist.

@yue4u yue4u force-pushed the add-pub-mod-option-for-unlinked-files branch from a74e1fd to 735c37c Compare April 14, 2021 02:55
@yue4u yue4u force-pushed the add-pub-mod-option-for-unlinked-files branch from 735c37c to bec89af Compare May 6, 2021 18:19
Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

This seems good to go, thanks!

bors d+

@bors
Copy link
Contributor

bors bot commented May 17, 2021

✌️ rainy-me can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@yue4u yue4u force-pushed the add-pub-mod-option-for-unlinked-files branch from bec89af to 6112376 Compare May 17, 2021 23:06
@yue4u yue4u force-pushed the add-pub-mod-option-for-unlinked-files branch from 6112376 to e0b01f3 Compare May 17, 2021 23:12
@yue4u
Copy link
Contributor Author

yue4u commented May 17, 2021

I resolved recent conflicts but think it's better to run the github workflow before r+?

@yue4u
Copy link
Contributor Author

yue4u commented May 18, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented May 18, 2021

@bors bors bot merged commit e3d0d89 into rust-lang:master May 18, 2021
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.

For unlinked files, add option to add "pub mod" instead of just "mod"
2 participants