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

feat: Improve type inference for Argv #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goisaki
Copy link

@goisaki goisaki commented Oct 12, 2024

This PR improves the type of Argv using generics.

Changes

  • Removed internal Default type
  • Added new internal Simplify type utility to keep the IDE hover type display clean (from type-fest under CC0-1.0)
  • BREAKING: Changed mri function generic
  • BREAKING?: Changed the exported Argv type with compatibility

Demo

After:
Screenshot 2024-10-12 at 10 43 09
Before:
Screenshot 2024-10-12 at 10 44 18

@goisaki
Copy link
Author

goisaki commented Oct 12, 2024

@goisaki
Copy link
Author

goisaki commented Oct 12, 2024

And note that this is an “ideal” type for convenience, but may differ from the actual.

Screenshot 2024-10-12 at 10 54 55

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2024

Hi there,

This is definitely breaking, and so I wouldn't be able to merge it as is, but it's prompted me to improve the type inference. Will update shortly

@goisaki
Copy link
Author

goisaki commented Oct 12, 2024

Thanks, @lukeed!
If I am allowed to do that implementation, I will make a version that is compatible. Or should I close the PR?

@lukeed
Copy link
Owner

lukeed commented Oct 12, 2024

I did it already, while adding options.default support. I will add options.alias support later, as it's more complicated.

Screen Shot 2024-10-12 at 1 36 23 AM

This updated version still allows users to pass in their own T value (as an override), to ensure/preserve non-breaking type behavior:

Screen Shot 2024-10-12 at 1 38 05 AM

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.

2 participants