feat: added an map api for pathCompleter #5940
Conversation
Gmin2
force-pushed
the
map-api-PathCompleter
branch
2 times, most recently
from
f547e3c to
51a319c
Compare
| .map(|candidate| { | ||
| let path = Path::new(candidate.get_value()); | ||
| let cargo_toml_path = path.join("Cargo.toml"); | ||
| let has_cargo_toml = cargo_toml_path.exists(); | ||
|
|
||
| if has_cargo_toml { | ||
| candidate | ||
| .help(Some("Addable cargo package".into())) | ||
| .display_order(Some(0)) | ||
| } else { | ||
| candidate.display_order(Some(1)) | ||
| } |
There was a problem hiding this comment.
Please demonstrate this feature when there is a user-provided directory filter and a map is used.
There was a problem hiding this comment.
And/or PathCompleter::file is being used with map
Gmin2
force-pushed
the
map-api-PathCompleter
branch
from
51a319c to
d946106
Compare
Gmin2
force-pushed
the
map-api-PathCompleter
branch
2 times, most recently
from
036b441 to
aea7818
Compare
| #[test] | ||
| fn suggest_value_path_with_filter_and_mapper() { |
There was a problem hiding this comment.
The intention of the split commits was for this test to overwrite the suggest_value_path_without_mapper, showing the change in behavior. Only the call to map and output should change in this commit.
There was a problem hiding this comment.
All test additions should be done in the first commit
There was a problem hiding this comment.
So @epage, i should do that same as i did for suggest_value_path_with_mapper (in the first commit showing the behaviour without mapper and then showing the behaviour with mapper right ?)
Gmin2
force-pushed
the
map-api-PathCompleter
branch
2 times, most recently
from
8bb3c50 to
cce8d49
Compare
Gmin2
force-pushed
the
map-api-PathCompleter
branch
from
cce8d49 to
d3ef660
Compare
| .filter(|path| { | ||
| path.file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .map_or(false, |name| name.starts_with('c')) |
There was a problem hiding this comment.
If you ran clippy on this, it would complain about the use of map_or. CI isn't catching it because we aren't running it on unstable features from non-default packages.
| .map_or(false, |name| name.starts_with('c')) | ||
| }) | ||
| .map(|candidate| { | ||
| if candidate.get_value().to_string_lossy().contains("c_dir") { |
There was a problem hiding this comment.
- These two are filtering under different conditions
- Logically, it feels odd that we're mapping on something that was filtered out. We shouldn't have to re-
filterinside themap.
There was a problem hiding this comment.
hmm... so i am thinking of filtering first if its a directory and then the mapping if it contains c_dir
thats seem correct right?
There was a problem hiding this comment.
map should only applied to content that matched filter.
| } else { | ||
| candidate.display_order(Some(1)) |
There was a problem hiding this comment.
Why this this being done? display order should be taken care of
There was a problem hiding this comment.
the display_order in the else branch ensures that c_dir comes first, if i dont include display_order() in the else branch it does not sort it properly
---- expected: tests/testsuite/engine.rs:1232:9
++++ actual: In-memory
1 - c_dir/ Addable cargo package
2 - d_dir/∅
1 + .
2 + d_dir/
3 + c_dir/ Addable cargo package∅
There was a problem hiding this comment.
If we filtered out d_dir, then it should be last. If it isn't, something else is going on and should be investigated and fixed.
A
mapAPI toPathCompleterand added its test, themapmethod allows customization of completion candidates after filtering, enabling use cases like highlighting directories, labeling etc