-
Notifications
You must be signed in to change notification settings - Fork 224
flag deprecated exports in autoimport and move to bottom
#1694
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
Changes from 2 commits
856a4ec
d2d5ba9
f799c78
187e0d7
7470c82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| use std::cmp::Ordering; | ||
| use std::cmp::Reverse; | ||
| use std::collections::BTreeMap; | ||
|
|
||
|
|
@@ -1676,7 +1677,9 @@ impl<'a> Transaction<'a> { | |
| let error_range = error.range(); | ||
| if error_range.contains_range(range) { | ||
| let unknown_name = module_info.code_at(error_range); | ||
| for handle_to_import_from in self.search_exports_exact(unknown_name) { | ||
| for (handle_to_import_from, export) in | ||
| self.search_exports_exact(unknown_name) | ||
| { | ||
| let (position, insert_text, _) = insert_import_edit( | ||
| &ast, | ||
| self.config_finder(), | ||
|
|
@@ -1686,7 +1689,12 @@ impl<'a> Transaction<'a> { | |
| import_format, | ||
| ); | ||
| let range = TextRange::at(position, TextSize::new(0)); | ||
| let title = format!("Insert import: `{}`", insert_text.trim()); | ||
| let mut title = format!("Insert import: `{}`", insert_text.trim()); | ||
|
|
||
| if export.deprecation.is_some() { | ||
| title.push_str(" (deprecated)"); | ||
| } | ||
|
|
||
| code_actions.push((title, module_info.dupe(), range, insert_text)); | ||
| } | ||
|
|
||
|
|
@@ -1709,7 +1717,16 @@ impl<'a> Transaction<'a> { | |
| _ => {} | ||
| } | ||
| } | ||
| code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| title1.cmp(title2)); | ||
|
|
||
| // Sort code actions: non-deprecated first, then alphabetically | ||
| code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| { | ||
| match (title1.contains("deprecated"), title2.contains("deprecated")) { | ||
| (true, false) => Ordering::Greater, | ||
| (false, true) => Ordering::Less, | ||
| _ => title1.cmp(title2), | ||
| } | ||
| }); | ||
|
Comment on lines
+1741
to
+1747
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also feels very non-idiomatic...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we sort within this function, we should rename it to maybe instead of this sort, we add a sort_key to code_actions. when we notice deprecated and add it, we put a sort-key that will put it at the bottom. then in this function, we can just sort by key once at the end. this will let us add other things to sort by in the future. we don't need to return the sort_key, so we can map it out after/during sorting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did the rename! I'm not quite sure how to implement the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the code_actions.sort_by call, we would do something like this: this means all the logic of sorting can be confined to when we add the code action: (sorry this removes the other suggestion of insert import deduplication unless you can figure out a clean way to keep it)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure how to do this cleanly. Because you'll also still want to sort by If you think it is better to use the |
||
|
|
||
| Some(code_actions) | ||
| } | ||
|
|
||
|
|
@@ -2934,11 +2951,11 @@ impl<'a> Transaction<'a> { | |
| (result, is_incomplete) | ||
| } | ||
|
|
||
| pub fn search_exports_exact(&self, name: &str) -> Vec<Handle> { | ||
| pub fn search_exports_exact(&self, name: &str) -> Vec<(Handle, Export)> { | ||
| self.search_exports(|handle, exports| { | ||
| if let Some(export) = exports.get(&Name::new(name)) { | ||
| match export { | ||
| ExportLocation::ThisModule(_) => vec![handle.dupe()], | ||
| if let Some(export_location) = exports.get(&Name::new(name)) { | ||
| match export_location { | ||
| ExportLocation::ThisModule(export) => vec![(handle.dupe(), export.clone())], | ||
| // Re-exported modules like `foo` in `from from_module import foo` | ||
| // should likely be ignored in autoimport suggestions | ||
| // because the original export in from_module will show it. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.