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

Unify Glob arguments into one Argument #155

Merged
merged 16 commits into from
May 1, 2023
Merged

Unify Glob arguments into one Argument #155

merged 16 commits into from
May 1, 2023

Conversation

KP64
Copy link
Contributor

@KP64 KP64 commented Apr 27, 2023

This comes at a degradation of usage friendliness.
Before:

erd -- --pattern <Pattern> --(glob | iglob)

After:

erd -- --pattern <Pattern> --glob (sensitive | insensitive)

It is worth it, though, since it is now harder to mess up the Argument checking in the Source Code.
Enums are exhaustive, after all.

The usage degradation could be minimized by using a shortcut for “--glob”. “-G” or “-g” for example.

KP64 and others added 4 commits April 27, 2023 19:36
The Glob enum has been introduced in order to get rid of duplicated settings. This makes the Checking of the provided arguments easier in source code since Enums are Exhaustive. This comes at a minimal usage friendliness degradation since now it would be needed to specify "--glob insensitive" instead of "--iglob". Even then I think it is worth it :3
@solidiquis
Copy link
Owner

Hmm I like where this is going but I feel like --glob should by default use case-sensitive searching without you needing to specify that it's case-sensitive. This is the behavior in similar programs such as ripgrep and fd and I wouldn't want to stray too far from what they're doing. What do you think?

src/render/context/mod.rs Outdated Show resolved Hide resolved
@KP64
Copy link
Contributor Author

KP64 commented May 1, 2023

Hmm I like where this is going but I feel like --glob should by default use case-sensitive searching without you needing to specify that it's case-sensitive. This is the behavior in similar programs such as ripgrep and fd and I wouldn't want to stray too far from what they're doing. What do you think?

I've had this thought too, but I don't really know how to do it, even after looking into it.
I've looked into the documentation of clap and couldn't find anything. This leaves me wondering if it is even possible.
Could you provide me with a reference on how to do it (if even possible)?

fd has implemented their Glob as a boolean, just like we did at the beginning.

#[arg(
  long,
  short = 'g',
  conflicts_with("fixed_strings"),
  help = "Glob-based search (default: regular expression)",
  long_help
)]
pub glob: bool,

KP64 added 3 commits May 1, 2023 16:03
While Fixing #158 I've gone ahead and implemented other sorting methods in advance.

#158
The command macros for the Context struct now use the Cargo.toml values of the packages, since they are the same. The will be no need to change the "version" in both, the mod.rs and the Cargo.toml file, when the version is bumped up.
@solidiquis
Copy link
Owner

I've had this thought too, but I don't really know how to do it, even after looking into it.

I'm not really sure off the top of my head how to do this nicely with clap's derive API. We might have to do some manual parsing of the argument which I'm not much a fan of.

One argument i found for keeping iglob and glob is to make use of argument grouping like so. The group keyword argument might allow us to have iglob and glob be mutually exclusive with one another. Let me know what you think.

@KP64
Copy link
Contributor Author

KP64 commented May 1, 2023

Seems good to me 👍

@KP64
Copy link
Contributor Author

KP64 commented May 1, 2023

Should I close this PR then, or adjust the Code to your suggestion?

@KP64 KP64 closed this May 1, 2023
@KP64 KP64 reopened this May 1, 2023
@solidiquis
Copy link
Owner

If you wouldn't mind changing to my suggestion that'd be great. I like some of the other code refactors in this PR :]

@KP64
Copy link
Contributor Author

KP64 commented May 1, 2023

If you wouldn't mind changing to my suggestion that'd be great. I like some of the other code refactors in this PR :]

Alright, everything should be ready to go now.
One last thing btw. I've added new sorting methods (in case you haven't seen). Truth be told, I forgot to switch branches and didn't realize that these changes went into this PR, haha.
#158 Should be closed by these Changes.

@solidiquis
Copy link
Owner

Oh nice! Thanks for taking care of #158! Much appreciated. Would you mind just fixing the merge conflicts? Once that's done and I run the workflows I'll get this merged in after breakfast :D

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging in a bit! Thanks for your help as always!

@solidiquis solidiquis merged commit b6ce790 into solidiquis:master May 1, 2023
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.

2 participants