-
-
Notifications
You must be signed in to change notification settings - Fork 104
Dependency cli parsing #673
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
base: master
Are you sure you want to change the base?
Conversation
I think I'd still go the other way around: from a CLI arg that directly targets a resolver and have it parse the URL into a dependency, and fallback to iterate each resolver to parse the URL for known hosts when no CLI arg is specified, and fail asking for an explicit resolver when we can't know what the remote is. |
I would go the other way: encode the full information in a single URI. The implementation of different strategies is quite simple: We parse the value as a URI and route it to a resolver-specific parser based on the scheme (e.g. If the URI scheme does not indicate a specific resolver (e.g. |
For GitHub, from the clone menu we should support
I want to support the most common format the user can copy paste directly. And I want to preserve as much as possible user intent. That's why I don't want the I saw mentions of Thanks for the feedback. |
Oh and should I move parsing and rendering to a DependencyDefinition type to avoid having these changes in dependency but not used in all places? |
We could probably turn the named tuple return type from |
20d0567
to
eafca30
Compare
I think I incorporated all of the feedback. |
expect_parses("github:Foo/[email protected]", "github", "Foo/Bar", VersionReq.new("~> 1.2.3")) | ||
|
||
# GitHub urls | ||
expect_parses("https://github.com/foo/bar", "github", "foo/bar", Any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I'm wondering about why this gets normalized to github
resolver instead of "git", "https://github.com/foo/bar"
.
Both options are valid. So we only need to decide which one we want to pick.
I suppose the reason for github
is that it's more concise? That's nice but not strictly necessary.
git
would be closer to the original input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me closer to the intent is to have github: foo/bar
, because I'm assuming the user is copy-pasting the url in the browser. We do preserve the format with trailing .git as those are copied from the clone popup.
At some point maybe it's worth configuring shards to map all github source to be resolved in some specific way, but is a separate story.
Related to #672, this PR adds the logic to parse dependencies from the cli. Still not used, but worth discussing syntax and implementation.
The
Dependency
class unfortunately is not fit to generate theshard.yaml
. Resolver key and source are normalized. I created a separate type.The added spec shows the various syntax supported.
To create a proper dependency, with the shard name, we need to actually fetch the shard to resolve that. This is done in
DependencyDefinition.from_cli
while theDependencyDefinition.parts_from_cli
is just the parsing without side effects.In general
{resolver}:{source}
syntax is supported, but there are some shorthands for convenience.git@...
github:foo/bar
github:Foo/[email protected]
(which will mean~> 1.2.3
)https://github.com/foo/bar
and other known hosts like gitlab, bitbucket, and codeberghttps://github.com/Foo/Bar/commit/000000
https://github.com/Foo/Bar/tree/v1.2.3
(which is interpreted as a tag since it's prefixed withv
)https://github.com/Foo/Bar/tree/some/branch
Some of these are maybe controversial, feel free to discuss.
If you want to play around you can use the following code.
which will output