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

Filepath select #138

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

anwarhahjjeffersongeorge

Hi. I tried to make a file path selector that:

  • Is its own prompt (PathSelect) with optional configuration (PathSelectPrompt) gated behind the path feature
  • Is OS-agnostic,
  • Uses the fs_err crate where possible for more detailed diagnostics,
  • Allows starting from a custom file path start_path_opt (defaulting to current process directory or root path if current process directory is unavailable)
  • Allows selection of one or multiple files with configuration option select_multiple,
  • Allows selecting (with select_multiple enabled) files from more than one directory,
  • Allows filtering of selectable file types by directory, extension, or a combination of multiple filters selection_mode,
  • Allows following/showing symbolic links with configuration option show_symlinks.
  • Allows showing hidden files with configuration option show_hidden,
  • Uses Rust's std::path and std::ffi::OsStr for portability instead of trying to figure out whether to convert paths to strings, and
  • Includes an example (path_select.rs).

Deficiencies

It does not:

  • Deal with permissions (it's possible, but I didn't know if it was a good idea),
  • Use existing prompts, although it is based heavily on the MultiSelect prompt code,
  • Handle formatting very aesthetically,
  • Support certain types of hidden files as I neither have a windows machine for testing nor understand how it works :(,
  • Support user-provided custom option filtering beyond selecting acceptable file types using the PathSelectionMode type ,
  • Support user-provided validation functions,
  • Show relative paths (there are crates to do this, but they seem to have some open issues), or
  • Have very many tests. I didn't know how you wanted tests written. For what it's worth, everything passes when I run 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.

@mikaelmello
Copy link
Owner

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.

@anwarhahjjeffersongeorge
Copy link
Author

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.

Copy link
Owner

@mikaelmello mikaelmello left a 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

inquire/src/prompts/path_select/action.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/action.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/action.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
#[derive(Clone, Eq, PartialEq)]
pub enum PathSelectionMode<'a> {
/// The user may pick a file with the given (optional) extension
File(Option<&'a str>),
Copy link
Owner

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

Copy link
Contributor

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 :)

Copy link
Contributor

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.

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?

inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/prompt.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
Comment on lines 28 to 30
/// Prompt for choosing one or multiple files.
///
/// The user can
Copy link
Owner

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

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?

inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/mod.rs Outdated Show resolved Hide resolved
Comment on lines 7 to 17
/// 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>>),
}
Copy link
Owner

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

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.

inquire/src/prompts/path_select/modes.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/modes.rs Outdated Show resolved Hide resolved
inquire/src/prompts/path_select/prompt.rs Outdated Show resolved Hide resolved
Comment on lines +522 to +526
if let Some(parent) = current_path.parent() {
*current_path = parent.to_path_buf();
*data_needs_refresh = true;
*cursor_index = 0;
}
Copy link
Owner

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",
    },
)

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?

@mikaelmello
Copy link
Owner

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

@Barre
Copy link

Barre commented Aug 1, 2023

Hi,

I am also interested in this.

Did this go anywhere? Any way I can help?

@anwarhahjjeffersongeorge
Copy link
Author

@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,
@anwarhahjjeffersongeorge
Copy link
Author

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.

Just let me know how you'd like me to reorganize it that would work for you.

Also, obligatory sorry for taking too long on this round of reviews

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.

@DaRacci
Copy link

DaRacci commented Feb 8, 2024

any progress with this?

@anwarhahjjeffersongeorge
Copy link
Author

anwarhahjjeffersongeorge commented Feb 8, 2024

any progress with this?
@DaRacci
@mikaelmello Said above they wanted me to reorganize or break this down but I'm not sure what that meant, so I'm waiting on that before doing more with this.

@mikaelmello
Copy link
Owner

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

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.

5 participants