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

Better Generic Support For ISCs #1706

Open
wants to merge 1 commit into
base: v2-maint
Choose a base branch
from

Conversation

danhunsaker
Copy link

@danhunsaker danhunsaker commented Mar 18, 2023

What type of PR is this?

  • feature

What this PR does / why we need it:

  • Allow generics to be set as their Set() string in the config
  • Allow converting internal ISC (InputSourceContext) types to Generics when:
    • The data used to define the Generic can be expressed in JSON
    • The target Generic implements FromJson([]byte) error
    • The source ISC implements Json(name string) ([]byte, error)
  • Update existing ISC implementations to also implement Json(name string) ([]byte, error)

And of course, there are tests for the new features.

As to why, the internal ISCs don't actually support Generics, as they don't know about these types to be able to deserialize into them directly. Proper support requires the ability to coerce the intermediate types into a Generic type. This can be done one of two ways - by using a string equivalent as you would on the CLI, or by implementing methods that can handle a JSON intermediate version on the Generic itself.

Given:

	app.Flags = []cli.Flag{
		altsrc.NewGenericFlag(&cli.GenericFlag{
			Name:   "location",
			Usage:  "where to connect",
			Value:  HostPort{},
		}),
		// ...
	}

The following examples are equivalent for the sample HostPort type added in this PR to the altsrc tests:

location: localhost:33
location:
  host: localhost
  port: 33

Third-party code that implements its own InputSourceContext may already support storing Generics in its internal structure, in which case the new code paths won't be hit, rendering full BC.

Testing

Tests were updated in the code to cover the new scenarios.

Release Notes

Better Generic support when using an InputSourceContext

@danhunsaker danhunsaker requested a review from a team as a code owner March 18, 2023 20:22
@danhunsaker danhunsaker force-pushed the feat/map-generics branch 2 times, most recently from 098c58a to be6cc47 Compare March 18, 2023 23:47
- Allow generics to be set as their Set() string in the config
- Allow converting internal ISC types to Generics when:
  - The data used to define the Generic can be expressed in JSON
  - The target Generic implements `FromJson([]byte) error`
  - The source ISC implements `Json(name string) ([]byte, error)`
- Update existing ISC implementations to also implement
  `Json(name string) ([]byte, error)`

And of course there are tests for the new features.
@dearchap
Copy link
Contributor

@danhunsaker v2 is only in maintenance mode. You do realise that v3/main branch has support for generics already ?

@danhunsaker
Copy link
Author

danhunsaker commented Mar 19, 2023

v3 isn't released/usable/documented yet. I imagine this will need to be forward-ported to v3 as well at some point, but until it's actually released, I can't use it in my own apps. Thus targeting v2 for the moment.

Plus, this PR is specifically for handling of Generics as loaded from config files or other alternate InputSourceContexts. Which v3 doesn't seem to have anymore. So not exactly relevant there.

@danhunsaker danhunsaker changed the title Better Generic Support For ISCs v2 - Better Generic Support For ISCs Mar 19, 2023
@meatballhat meatballhat added kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 labels Mar 26, 2023
@meatballhat meatballhat added this to the Release 2.x milestone Mar 26, 2023
@meatballhat meatballhat changed the title v2 - Better Generic Support For ISCs Better Generic Support For ISCs Mar 26, 2023
@meatballhat
Copy link
Member

@danhunsaker Thank you again for your patience! I've got much the same feedback as in #1702. How would you like to handle next steps?

@meatballhat meatballhat added the status/waiting-for-response Waiting for response from original requester label May 1, 2023
@bartekpacia
Copy link
Contributor

Hey, @danhunsaker. Thank you for this PR.

Some merge conflicts showed up, and as Dan explained above, we're very reluctant to accept new features into v2 (much smaller PRs were getting rejected because of this policy).

Now, I'm a new maintainer and new to this codebase, so I'm not yet sure if it makes sense to re-target this PR against https://github.com/urfave/cli-altsrc.

I'm just trying to see what's the state that this PR is in. I don't like if they live for long, because they rot.

Again, thanks for all the work.

@abitrolly
Copy link
Contributor

Extending code surface to +416 new lines for a maintenance project is also a security risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/feature describes a code enhancement / feature request status/waiting-for-response Waiting for response from original requester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants