-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
feat(parser): Introduce clap based parser #197
Conversation
4e67b3e
to
66da265
Compare
I have cloned and built this fork. This is indeed a solid start 👍 After a couple fixes, this will be ready for a deeper look, and we can discuss things like the -h flag and duplicate args (if that cannot be fixed). Thanks for your work here 😄 @cafkafk I"m going to help him get this ready for review... Still got some things to work on. @MartinFillon I would suggest just re-marking this as a draft for now... No need to rush, it's a huge change. |
Tasks:
It is starting to look like something |
14fa6d1
to
9d88912
Compare
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 wasn’t convinced by the idea at first, but I must admit it’s more readable and it allows us to get rid of a ton of boilerplate! And we don’t need to test code too much since clap is already tested & trustworthy. So yeah, nice work so far!
f5e8984
to
6dc3c87
Compare
Most of the work is done, only part lasting is finding bugs. |
6dc3c87
to
c0195a7
Compare
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 wonder if we shouldn’t drop the strict mode, it seems complicated for a very marginal gain, but maybe some people use it??
Anyway, hope the feedback isn’t too heavy, it looks great overall.
I went through some of the code as well, @MartinFillon we spoke about this, i'll be pushing my changes with tests probably to your fork maybe tomorrow, but I had a similar thought...
It feels like it would simplify a lot of what's going on. Are there any other thoughts on this? @cafkafk | @gierens ? This is a pretty monumental change here... I wrote a .tape file with just about every permutation of command that can/should be run, it just needs to be refined a little and I'm going to write a script to run it against this fork and the main branch, so we can prove the output will remain the same. between the two. I feel like there is really no substitution for just using it though.. So I will also temp remove eza from my path and use this fork for the next couple days and see if i run into any bugs personally. ALSO: I'll wait till you resolve the current review before I add my official review on here. |
I'm not actually sure I fully understand the "strictness", my assessment is solely based on what the man page says: "EXA_STRICT" tells eza to error when incompatible options are given, right? If so, I'm a little baffled. I mean I would expect a program to always fail when presented with parameters that are mutually exclusive. I think most people would, which is probably also why clap doesn't allow it either. So, the way I see it the "strict" mode (or its by default absence) is more of a trap or bug than a feature and should be removed in general. If it on top of that makes this PR easier then please go ahead and align this with claps behavior. |
yeah u got it from what i understand, If you put it in the context of like my alias in my alias ls = eza -hlA --no-user --no-permissions --git but if I were to execute ls and then pass a conflicting flag, the command would fail instead of just work "the best it could" to come up with a command that it determines I'm most likely asking for. I cannot see where this isn't the behavior you would want tbh.. but someone may very well have a reason for wanting it to fail, outside of testing? |
I actually meant that the normal "non-strict" behavior is what should be removed, because imho a CLI program should fail immediately when flags are incompatible and not start guessing around. |
I understand what you meant, I just think it might be problematic... In that case you couldn't really have an alias for your default behavior and comfortably pass additional flags on the command line, because if any of them conflict or duplicate or just don't go together (just because of how many options there are), you would get an error instead of the default behavior. Which I think would happen quite frequently, they may often pass a Long option flag to a -1 view, and currently it will just display the -1 view instead of throwing an error. If it wasn't used in alias's so extensively I dont think this would be a problem, but I imagine its a big reason why Ogham wrote it the way he did. I could definitely be wrong, this is just my impression of it. |
Yeah sure, I agree that this is probably used quite often in aliases. But then I kinda have to argue for the strict mode now :D |
So strict mode stays an option 👍 or are you voting do away with normal mode? o_O |
No, I mean that's just my opinion and I wouldn't wanna impose that on everyone, so I would just vote for let's keep strict mode if possible :) |
852168f
to
6db6637
Compare
Signed-off-by: Christina Sørensen <christina@cafkafk.com>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Replace "cat + grep + head + awk + tr" by one awk command. That's also avoid the "useless cat" case : https://github.com/koalaman/shellcheck/wiki/SC2002 Co-authored-by: gierens <sandro@gierens.de> Co-authored-by: Gardouille <gardouille@gmail.com>
This also avoid "useless cat" and allow `just` command to build manpages. Resolves eza-community#458 Co-authored-by: gierens <sandro@gierens.de> Co-authored-by: Gardouille <gardouille@gmail.com>
Bumps [libc](https://github.com/rust-lang/libc) from 0.2.148 to 0.2.149. - [Release notes](https://github.com/rust-lang/libc/releases) - [Commits](rust-lang/libc@0.2.148...0.2.149) --- updated-dependencies: - dependency-name: libc dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [DeterminateSystems/nix-installer-action](https://github.com/determinatesystems/nix-installer-action) from 4 to 5. - [Release notes](https://github.com/determinatesystems/nix-installer-action/releases) - [Commits](DeterminateSystems/nix-installer-action@v4...v5) --- updated-dependencies: - dependency-name: DeterminateSystems/nix-installer-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
This var will override any --git or --git-repos
Updated the man page to include: - --dereference - --almost-all - --help - --version Additionally, added the following flags to the CLI --help information: - --dereference - --almost-all Updated shell completions.
Signed-off-by: Sandro-Alessio Gierens <sandro@gierens.de>
Sorry for commit duplication xd |
poke @cafkafk how are we doing in terms of powertes ? |
I think this PR took a lot of time. Is what you have achieved so far sufficient? When is it expected to end? |
Well this PR is stuck by powertests, which is another repo of eza, on which @cafkafk is supposed to work and I am awaiting her heads up in order to fully finish and then merge clap-based parser |
Sadly, none of us are paid, and writing an extensive testing framework and integrating it into an open source project isn't easily the kind of feature that gets the most contributors inspired to contribute to, as it isn't as flashy as adding new features. It's a long, hard process, but when this groundwork is laid, it will serve as a very strong foundation for not only this, but many other extensive changes to come. I'm already working many hours just to review and maintain eza, and while a part of me wanna rush to finish this, open source is a marathon, the last thing I would want for eza is maintainership burnout, leaving us right where we started with exa. Slowly and steadily we'll get there, we are building something that will support eza development for years, it's important to get it right. I've seen the result of other projects rushing for these things, and years later they're still stuck with the tech debt they introduced, and everyone hates it. And rewriting it when it has had some years to really get stuck as a fundamental part of the project is extremely painful. We already banished vagrant, which wasn't easy either. We've done hard things before, and we'll do it again. |
This commits is a copy of my local branch in order to help people contribute to it. This refs #197
This commits is a copy of my local branch in order to help people contribute to it. This refs #197
This commits is a copy of my local branch in order to help people contribute to it. This refs #197
please now use #638 PR |
Introduce a clap based arg parsing