Skip to content

Conversation

@dearchap
Copy link
Contributor

@dearchap dearchap commented Nov 17, 2024

Fixes #10

@dearchap dearchap requested a review from meatballhat November 17, 2024 13:22
@YvanDaSilva
Copy link

@dearchap This is awesome!
IMHO, it's going to make programs that depend on urface/cli lighter and better.

@meatballhat
Copy link
Member

Overall, I am in favor of this direction 🎉 That being said, I haven't worked with nested modules enough to know the trade-offs. I think that given we're in a prerelease phase anyway, I would advocate for each module being in its own repository.

This could also include addressing the awkwardness of namespace collisions so that instead of having to deal with package import aliases, the declared package names could be like altsrcyaml altsrcjson.

@abitrolly
Copy link
Contributor

This could also include addressing the awkwardness of namespace collisions so that instead of having to deal with package import aliases, the declared package names could be like altsrcyaml altsrcjson.

What kind of collision awkwardness is expected? I find import like altsrcyaml already awkward.

@dearchap
Copy link
Contributor Author

Here's a good example. https://github.com/knadh/koanf

@dearchap
Copy link
Contributor Author

In keeping with koanf example above I have raised a PR in cli

urfave/cli#2009

This allows map value sources to plugin easily without having to do custom lookups. Once that PR is approved the providers here neednt import the root urfave/cli-altsrc and then each provider can plugin to cli directly. How the provider retrieves its information and unmarshals can be left to its implementation

@dearchap dearchap merged commit ed4233e into urfave:main Dec 27, 2024
0 of 9 checks passed
@mrueg
Copy link
Contributor

mrueg commented Jan 22, 2025

Would you be able to create a new release with these changes in place?

@@ -0,0 +1,20 @@
module github.com/urfave/cli-altsrc/json
Copy link
Contributor

Choose a reason for hiding this comment

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

@dearchap should these be also on v3 or do you want to keep them loosely coupled?

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.

feature: provider based packaging

5 participants