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

Update to clap 4 + rework to declarative style #32

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

Conversation

tonky
Copy link
Contributor

@tonky tonky commented Oct 9, 2022

And here's something no-one asked for but i was interested in: Clap 4!

Saw recent clap release, also never had a chance to try it, so i decided to give it a go.

I think declarative style is easier to read(and args parsing is more explicit). Also i took some liberties with search and list args for filtering, turning them into discriminated union(since they are mutually exclusive).

This changes the interface a bit, but i though about it as a suggestion - i'll change it back to the way it works now, if you think it's not worth it.

As for the rest of the changes - same story, feel free to reject it if you don't need or like it, it was mostly done as a personal excercise :)

@hwittenborn
Copy link
Member

Hey, thanks again for getting another MR done, it's really appreciated :). I think this declarative style probably will have some benefits in terms of grouping data together, I'm definitely not close to rejecting this change. I'm doing some stuff at church today though so I won't be available until later this afternoon, but I'll get this checked out later today when I get some time.

Copy link
Member

@hwittenborn hwittenborn left a comment

Choose a reason for hiding this comment

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

I still need to run your code locally to verify everything, but most of your changes look great! Just had a few things I needed changed and had questions about, and it should be good to go after that.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/style.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@tonky
Copy link
Contributor Author

tonky commented Oct 10, 2022

You're correct on all your comments, thanks! Fixed.

Also, while refactoring I may have gone a bit overboard:

  • implemented search in terms of list
  • user Itertools for unique_by in search/list results
  • added CachePackage.is_installed
  • resurrected Cache.pkglist - it's more useful to iterate/filter over, imo, instead of pkgmap
  • added Cache.search() for pkgname and pkgdesc
  • changed generate_pkginfo_entry to use pkgname directly
  • and some other minor fixes.

As per usual - feel free to comment on what should be reverted or reworked.

@tonky
Copy link
Contributor Author

tonky commented Nov 2, 2022

@hwittenborn - i can rebase to fresh master to get rid of conflicts, if this PR is still releveant?

@jrop jrop mentioned this pull request Nov 2, 2022
@hwittenborn
Copy link
Member

Yeah, I still want to get this merged @tonky. I implemented this derive style in some code for makedeb, and it's definitely a lot cleaner than the builder method.

Feel free to rebase it, let me know if you need any help if any conflicts arise too, I'll be around :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants