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

Make a PR upstream? #23

Open
InnocentZero opened this issue Oct 6, 2024 · 10 comments
Open

Make a PR upstream? #23

InnocentZero opened this issue Oct 6, 2024 · 10 comments

Comments

@InnocentZero
Copy link
Collaborator

Hopefully we should be able to make a PR upstream now that things are a lot more stable than before. @Magniquick wanted to try out the new repo, so we should probably have him as a tester and report changes.

A couple of other ideas:

  • Have a more appropriate message on triggering pacdef review so that people know it's still under work.
  • pacdef unmanaged can reduce visual clutter by not listing empty backends.
  • We still haven't tackled the issue of explicitly installed runtimes in flatpak. The fix should be easy enough.

Unfortunately I won't be able to contribute a lot/very frequently till the next year because of a very hectic semester at uni. Once again, thanks for your efforts on this project @ripytide !

@Magniquick
Copy link

tryna edit the AUR pacdef-git to get this running, do I no longer need to use --features arch anymore ?

@ripytide
Copy link
Owner

ripytide commented Oct 7, 2024

The AUR packages belong to @steven-omaha and point to the https://github.com/steven-omaha/pacdef rather than this fork so that won't work atm. You are correct in that you no longer need the --features arch flag due to us switching to command line for all backends and so every backend is now included by default.

@ripytide
Copy link
Owner

ripytide commented Oct 7, 2024

The easiest way to install this would probably be using cargo, perhaps: cargo install --git https://github.com/ripytide/pacdef might work.

@Magniquick
Copy link

Magniquick commented Oct 7, 2024

Trying it out, gotta say that the new group format with the indentation and the quotes and commas is a bit more annoying than the old toml used .
At the least a converter would be cool.
Also by default no groups applying also feels kinda backward imho, but it's a small edit, so either works.
Also, are pacdef group edit and other QoL commands in plans or is it not going to be implemented ?

@Magniquick
Copy link

Magniquick commented Oct 7, 2024

Also pacdef clean reports the entire package tree that would be removed, rather than the ones specifically marked installed, which for me makes it much harder to read.

@InnocentZero
Copy link
Collaborator Author

The old syntax was INI and not TOML. TOML allows for some really good and useful features which is why we went with this.

We can possibly write a converter script in python, and link to it in the README.

I agree with the point of no groups applying by default, we should change it.

The point about pacdef clean also makes sense. I'll hopefully be able to roll out a couple of changes tomorrow.

@ripytide
Copy link
Owner

ripytide commented Oct 8, 2024

I've opened #24 to fix all the mentioned improvements except the converter (can be done in a separate PR) and the pacdef clean suggestion:

Also pacdef clean reports the entire package tree that would be removed, rather than the ones specifically marked installed, which for me makes it much harder to read.

I'm not sure I understand this, could you provide an example or re-phrase it maybe?

@Magniquick
Copy link

I'm not sure I understand this, could you provide an example or re-phrase it maybe?

Basically it reports ALL the packages that would be uninstalled, rather than the ones that I've manually installed (for arch)
Say if I install foo, and it pulls bar as a dependency, It shows that both foo and bar will be removed, when I have only manually installed foo.
Since dependency trees grow really quickly, this makes the output effectively impossible to read.

@ripytide
Copy link
Owner

Ah that makes sense, thanks. I'll add a new cli option for the verbose behaviour --include-implicit but make it default to only show explicitly installed packages.

@ripytide
Copy link
Owner

Changes Made in #24. I'm going to suggest in steven-omaha/pacdef#92 if anyone wants to try out this version of pacdef.

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

No branches or pull requests

3 participants