-
Notifications
You must be signed in to change notification settings - Fork 74
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
Filepath select #138
base: main
Are you sure you want to change the base?
Filepath select #138
Conversation
I really like this. And thank you for the attention to detail on the PR description lol. Hopefully I'll find some time tomorrow for a full review. One important point that I'd bring up is that recently (like yesterday maybe) I've refactored prompts. Far from a revamp but significant enough that you might want to get ahead in porting this PR already. Honestly, I can also do that myself if you'd prefer, whatever works. |
I gave refactoring the prompts a shot. For now, I'll chill out on this until whenever you get a chance to look and let me know what needs improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the first half hour I had to not keep you waiting too much!
I still haven't reviewed much of the prompt implementation itself, nor could tinker with PathSelect as an user, but so far it looks pretty solid!!! Thanks for your fantastic work
#[derive(Clone, Eq, PartialEq)] | ||
pub enum PathSelectionMode<'a> { | ||
/// The user may pick a file with the given (optional) extension | ||
File(Option<&'a str>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeoRiether, is using Cow<'a, str>
here a good idea? From your comments on that another issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are pros and cons. Cow
in some cases does provide easier lifetime handling, but it also adds some complexity to the API, like the need to write File(Some("jpg".into()))
instead of File(Some("jpg"))
and some users saying "what the hell is a Cow?"
Maybe it's worth it? I didn't find clear guidelines about this online, but I'll look into it again later to try and come up with a more definitive answer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I feel like it would be nice to have a more flexible PathSelectionMode
. I don't see an easy way, for example, to integrate the ignore crate to list files respecting .gitignore
. If I have any ideas of how to actually implement this more flexible API, I'll make sure to post them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. I don't see an easy way, for example, to integrate the ignore crate to list files respecting
.gitignore
.
Hiya. I'm not sure what you're asking for here. Would it help if I tried to add globs or some kind of name-matching/name-excluding patterns? I looked at the ignore
crate, but I don't know if adding that dependency would be better than just using the regex
crate. Are you wanting to respect .gitignore
files in the ancestry of paths too?
More explicit documentation for navigation behavior Co-authored-by: Mikael Mello <[email protected]>
Fix my doc typo Co-authored-by: Mikael Mello <[email protected]>
Fix doc string typo Co-authored-by: Mikael Mello <[email protected]>
- Support user-selectable item sorting modes - Add a test for filtering files by extension
/// Prompt for choosing one or multiple files. | ||
/// | ||
/// The user can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to finish writing the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think. One thing I did not document was the divider. It is currently unused because I couldn't figure out how to make it work with the prompt — basically, I thought it would be less confusing with a visual divider between the current directory's entries and items selected from previous directories if any. Only I don't know how to insert a divider into the list or if that's possible, so maybe you can suggest something?
/// Different path selection modes specify what the user can choose | ||
#[derive(Clone, Eq, PartialEq)] | ||
pub enum PathSelectionMode<'a> { | ||
/// The user may pick a directory. | ||
Directory, | ||
/// The user may pick a file with the given (optional) extension. | ||
File(Option<&'a str>), | ||
/// The user can set gitignore rules from a file | ||
/// The user may pick multiple paths | ||
Multiple(Vec<PathSelectionMode<'a>>), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this can be made simpler for end-users. On a first glance, it's not quite intuitive how this selection mode behaves, with the recursive Multiple warranting more attention.
wdyt of:
accept enum, like JS's accept attribute: All, Only(String), Any(Vec)
Then PathSelectionMode
- Directory
- File(Accept)
- FileOrDirectory(Accept)
I still don't fee this is the best solution, but it feels more intuitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I suppose it was redundant the way it was. I tried implementing an inner enum (PathFilter
) that can accept or deny based on file extensions, stems, and matching functions. I also added some documentation tests for the different modes and tried to tighten up the documentation.
if let Some(parent) = current_path.parent() { | ||
*current_path = parent.to_path_buf(); | ||
*data_needs_refresh = true; | ||
*cursor_index = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this error when going up and up, not sure why as it seems you put the handling into place, but worth checking out
IO(
Os {
code: 2,
kind: NotFound,
message: "No such file or directory",
},
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to replicate this on Ubuntu Linux, but it was indeed a persistent problem before. Can you please let me know what OS you were on so I can try to write a test or something and figure it out?
I do feel bad about dumbing a lot of comments, but it is a very large feature :). I want it to be as good as possible before releasing, which today is basically the same as merging. I was thinking, we should break this down in smaller chunks and get the thing going more gradually, we can merge but not expose this on the published crate until we gradually get this to the end state. I think this would also allow us to work in parallel. Also, obligatory sorry for taking too long on this round of reviews |
Hi, I am also interested in this. Did this go anywhere? Any way I can help? |
@mikaelmello @Barre Hey there, I missed this before, but I'll take a look at these next weekend. |
Remove vim mode support Co-authored-by: Mikael Mello <[email protected]>
Formatting Co-authored-by: Mikael Mello <[email protected]>
Formatting Co-authored-by: Mikael Mello <[email protected]>
Prompt formatting Co-authored-by: Mikael Mello <[email protected]>
Remove optional starting path from instantiator Co-authored-by: Mikael Mello <[email protected]>
Remove raw prompt methods Co-authored-by: Mikael Mello <[email protected]>
- Eliminate vim_mode - Add documentation
Clarify PathSelectionMode, Eliminate strum dependency from PathSortingMode,
I was thinking, we should break this down in smaller chunks and get the thing going more gradually, we can merge but not expose this on the published crate until we gradually get this to the end state.
Just let me know how you'd like me to reorganize it that would work for you.
No problem. It's been like 110+ degrees where I live, so haven't exactly been rushing to turn the computer on and heat up the house. |
any progress with this? |
|
hey, I know I've been really dropping the ball on being responsive throughout inquire, life getting in the way as usual I really really want to see this merged, the main point holding it back is that this is a very significant prompt to offer, counter-intuitively. it's one of those things I'd like to polish and re-polish it before releasing, making sure the API is very intuitive and everything |
Hi. I tried to make a file path selector that:
PathSelect
) with optional configuration (PathSelectPrompt
) gated behind thepath
featurefs_err
crate where possible for more detailed diagnostics,start_path_opt
(defaulting to current process directory or root path if current process directory is unavailable)select_multiple
,select_multiple
enabled) files from more than one directory,selection_mode
,show_symlinks
.show_hidden
,std::path
andstd::ffi::OsStr
for portability instead of trying to figure out whether to convert paths to strings, andDeficiencies
It does not:
MultiSelect
prompt code,PathSelectionMode
type ,cargo test --all-features-
. It seems to work when I tried it on Ubuntu 23.04 and Mac OS 10.15.7.I am opening this PR mainly for feedback. I know the contributor's guide said to do the discussion stage first, but I needed this feature for a project and did not see that detail until after I made it. I saw some existing issues (#124, #63), and tried to go from there, favoring the strategy of 63 ( a new prompt type) with the features of 124 (as detailed above).
With all that in mind, I understand if this needs more work.