From 74186d3ae7265b691f00b562f802ed5bdcda5c89 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 2 Sep 2020 19:22:23 +0200 Subject: [PATCH] Tidy up tests and apply suggested changes --- crates/assists/src/utils/insert_use.rs | 179 +++++++++++++++---------- 1 file changed, 107 insertions(+), 72 deletions(-) diff --git a/crates/assists/src/utils/insert_use.rs b/crates/assists/src/utils/insert_use.rs index 8563b16abec..dbe2dfdcb1b 100644 --- a/crates/assists/src/utils/insert_use.rs +++ b/crates/assists/src/utils/insert_use.rs @@ -34,14 +34,15 @@ pub(crate) fn insert_use_statement( insert_use(position.clone(), make::path_from_text(path_to_import), Some(MergeBehaviour::Full)); } +/// Insert an import path into the given file/node. A `merge` value of none indicates that no import merging is allowed to occur. pub fn insert_use( where_: SyntaxNode, path: ast::Path, - merge_behaviour: Option, + merge: Option, ) -> SyntaxNode { let use_item = make::use_(make::use_tree(path.clone(), None, None, false)); // merge into existing imports if possible - if let Some(mb) = merge_behaviour { + if let Some(mb) = merge { for existing_use in where_.children().filter_map(ast::Use::cast) { if let Some(merged) = try_merge_imports(&existing_use, &use_item, mb) { let to_delete: SyntaxElement = existing_use.syntax().clone().into(); @@ -59,17 +60,24 @@ pub fn insert_use( let to_insert: Vec = { let mut buf = Vec::new(); - if add_blank == AddBlankLine::Before { - buf.push(make::tokens::single_newline().into()); + match add_blank { + AddBlankLine::Before => buf.push(make::tokens::single_newline().into()), + AddBlankLine::BeforeTwice => { + buf.push(make::tokens::single_newline().into()); + buf.push(make::tokens::single_newline().into()); + } + _ => (), } buf.push(use_item.syntax().clone().into()); - if add_blank == AddBlankLine::After { - buf.push(make::tokens::single_newline().into()); - } else if add_blank == AddBlankLine::AfterTwice { - buf.push(make::tokens::single_newline().into()); - buf.push(make::tokens::single_newline().into()); + match add_blank { + AddBlankLine::After => buf.push(make::tokens::single_newline().into()), + AddBlankLine::AfterTwice => { + buf.push(make::tokens::single_newline().into()); + buf.push(make::tokens::single_newline().into()); + } + _ => (), } buf @@ -83,8 +91,8 @@ fn try_merge_imports( new: &ast::Use, merge_behaviour: MergeBehaviour, ) -> Option { - // dont merge into re-exports - if old.visibility().map(|vis| vis.pub_token()).is_some() { + // don't merge into re-exports + if old.visibility().and_then(|vis| vis.pub_token()).is_some() { return None; } let old_tree = old.use_tree()?; @@ -115,8 +123,8 @@ pub fn try_merge_trees( let rhs_tl = rhs.use_tree_list()?; // if we are only allowed to merge the last level check if the split off paths are only one level deep - if merge_behaviour == MergeBehaviour::Last && use_tree_list_is_nested(&lhs_tl) - || use_tree_list_is_nested(&rhs_tl) + if merge_behaviour == MergeBehaviour::Last + && (use_tree_list_is_nested(&lhs_tl) || use_tree_list_is_nested(&rhs_tl)) { return None; } @@ -124,18 +132,15 @@ pub fn try_merge_trees( let should_insert_comma = lhs_tl .r_curly_token() .and_then(|it| skip_trivia_token(it.prev_token()?, Direction::Prev)) - .map(|it| it.kind() != T![,]) - .unwrap_or(true); + .map(|it| it.kind()) + != Some(T![,]); let mut to_insert: Vec = Vec::new(); if should_insert_comma { to_insert.push(make::token(T![,]).into()); to_insert.push(make::tokens::single_space().into()); } to_insert.extend( - rhs_tl - .syntax() - .children_with_tokens() - .filter(|it| it.kind() != T!['{'] && it.kind() != T!['}']), + rhs_tl.syntax().children_with_tokens().filter(|it| !matches!(it.kind(), T!['{'] | T!['}'])), ); let pos = InsertPosition::Before(lhs_tl.r_curly_token()?.into()); let use_tree_list = lhs_tl.insert_children(pos, to_insert); @@ -225,6 +230,7 @@ fn segment_iter(path: &ast::Path) -> impl Iterator + Cl #[derive(PartialEq, Eq)] enum AddBlankLine { Before, + BeforeTwice, After, AfterTwice, } @@ -278,7 +284,9 @@ fn find_insert_position( } // there is no such group, so append after the last one None => match last { - Some(node) => (InsertPosition::After(node.into()), AddBlankLine::Before), + Some(node) => { + (InsertPosition::After(node.into()), AddBlankLine::BeforeTwice) + } // there are no imports in this file at all None => (InsertPosition::First, AddBlankLine::AfterTwice), }, @@ -297,12 +305,14 @@ mod tests { #[test] fn insert_start() { check_none( - "std::bar::A", - r"use std::bar::B; + "std::bar::AA", + r" +use std::bar::B; use std::bar::D; use std::bar::F; use std::bar::G;", - r"use std::bar::A; + r" +use std::bar::AA; use std::bar::B; use std::bar::D; use std::bar::F; @@ -313,14 +323,16 @@ fn insert_start() { #[test] fn insert_middle() { check_none( - "std::bar::E", - r"use std::bar::A; + "std::bar::EE", + r" +use std::bar::A; use std::bar::D; use std::bar::F; use std::bar::G;", - r"use std::bar::A; + r" +use std::bar::A; use std::bar::D; -use std::bar::E; +use std::bar::EE; use std::bar::F; use std::bar::G;", ) @@ -329,29 +341,33 @@ fn insert_middle() { #[test] fn insert_end() { check_none( - "std::bar::Z", - r"use std::bar::A; + "std::bar::ZZ", + r" +use std::bar::A; use std::bar::D; use std::bar::F; use std::bar::G;", - r"use std::bar::A; + r" +use std::bar::A; use std::bar::D; use std::bar::F; use std::bar::G; -use std::bar::Z;", +use std::bar::ZZ;", ) } #[test] - fn insert_middle_pnested() { + fn insert_middle_nested() { check_none( - "std::bar::E", - r"use std::bar::A; + "std::bar::EE", + r" +use std::bar::A; use std::bar::{D, Z}; // example of weird imports due to user use std::bar::F; use std::bar::G;", - r"use std::bar::A; -use std::bar::E; + r" +use std::bar::A; +use std::bar::EE; use std::bar::{D, Z}; // example of weird imports due to user use std::bar::F; use std::bar::G;", @@ -361,17 +377,19 @@ fn insert_middle_pnested() { #[test] fn insert_middle_groups() { check_none( - "foo::bar::G", - r"use std::bar::A; + "foo::bar::GG", + r" +use std::bar::A; use std::bar::D; use foo::bar::F; use foo::bar::H;", - r"use std::bar::A; + r" +use std::bar::A; use std::bar::D; use foo::bar::F; -use foo::bar::G; +use foo::bar::GG; use foo::bar::H;", ) } @@ -379,17 +397,19 @@ fn insert_middle_groups() { #[test] fn insert_first_matching_group() { check_none( - "foo::bar::G", - r"use foo::bar::A; + "foo::bar::GG", + r" +use foo::bar::A; use foo::bar::D; use std; use foo::bar::F; use foo::bar::H;", - r"use foo::bar::A; + r" +use foo::bar::A; use foo::bar::D; -use foo::bar::G; +use foo::bar::GG; use std; @@ -399,18 +419,35 @@ fn insert_first_matching_group() { } #[test] - fn insert_missing_group() { + fn insert_missing_group_std() { check_none( "std::fmt", - r"use foo::bar::A; + r" +use foo::bar::A; use foo::bar::D;", - r"use std::fmt; + r" +use std::fmt; use foo::bar::A; use foo::bar::D;", ) } + #[test] + fn insert_missing_group_self() { + check_none( + "self::fmt", + r" +use foo::bar::A; +use foo::bar::D;", + r" +use foo::bar::A; +use foo::bar::D; + +use self::fmt;", + ) + } + #[test] fn insert_no_imports() { check_full( @@ -436,23 +473,12 @@ fn insert_empty_file() { } #[test] - fn adds_std_group() { - check_full( - "std::fmt::Debug", - r"use stdx;", - r"use std::fmt::Debug; - -use stdx;", - ) - } - - #[test] - fn merges_groups() { + fn merge_groups() { check_last("std::io", r"use std::fmt;", r"use std::{fmt, io};") } #[test] - fn merges_groups_last() { + fn merge_groups_last() { check_last( "std::io", r"use std::fmt::{Result, Display};", @@ -462,7 +488,7 @@ fn merges_groups_last() { } #[test] - fn merges_groups_full() { + fn merge_groups_full() { check_full( "std::io", r"use std::fmt::{Result, Display};", @@ -471,7 +497,7 @@ fn merges_groups_full() { } #[test] - fn merges_groups_long_full() { + fn merge_groups_long_full() { check_full( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", @@ -480,7 +506,7 @@ fn merges_groups_long_full() { } #[test] - fn merges_groups_long_last() { + fn merge_groups_long_last() { check_last( "std::foo::bar::Baz", r"use std::foo::bar::Qux;", @@ -489,7 +515,7 @@ fn merges_groups_long_last() { } #[test] - fn merges_groups_long_full_list() { + fn merge_groups_long_full_list() { check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", @@ -498,7 +524,7 @@ fn merges_groups_long_full_list() { } #[test] - fn merges_groups_long_last_list() { + fn merge_groups_long_last_list() { check_last( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, Quux};", @@ -507,7 +533,7 @@ fn merges_groups_long_last_list() { } #[test] - fn merges_groups_long_full_nested() { + fn merge_groups_long_full_nested() { check_full( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", @@ -516,7 +542,7 @@ fn merges_groups_long_full_nested() { } #[test] - fn merges_groups_long_last_nested() { + fn merge_groups_long_last_nested() { check_last( "std::foo::bar::Baz", r"use std::foo::bar::{Qux, quux::{Fez, Fizz}};", @@ -526,7 +552,7 @@ fn merges_groups_long_last_nested() { } #[test] - fn skip_merges_groups_pub() { + fn merge_groups_skip_pub() { check_full( "std::io", r"pub use std::fmt::{Result, Display};", @@ -535,22 +561,31 @@ fn skip_merges_groups_pub() { ) } - // should this be a thing? #[test] - fn split_merge() { + #[ignore] // FIXME: Support this + fn split_out_merge() { check_last( "std::fmt::Result", r"use std::{fmt, io};", - r"use std::fmt::Result; + r"use std::{self, fmt::Result}; use std::io;", ) } #[test] - fn merges_groups_self() { + fn merge_groups_self() { check_full("std::fmt::Debug", r"use std::fmt;", r"use std::fmt::{self, Debug};") } + #[test] + fn merge_self_glob() { + check_full( + "token::TokenKind", + r"use token::TokenKind::*;", + r"use token::TokenKind::{self::*, self};", + ) + } + fn check( path: &str, ra_fixture_before: &str,