Dependency cli parsing

[bcardiff]

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 the shard.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 the DependencyDefinition.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/Bar#1.2.3 (which will mean = 1.2.3)
• https://github.com/foo/bar and other known hosts like gitlab, bitbucket, and codeberg
• https://github.com/foo/bar#1.2.3 and other known hosts like gitlab, bitbucket, and codeberg
• ~~https://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 with v)~~
• ~~https://github.com/Foo/Bar/tree/some/branch~~
• relative and absolute paths

Some of these are maybe controversial, feel free to discuss.

If you want to play around you can use the following code.

require "./src/dependency_definition"

def t(s)
  d = Shards::DependencyDefinition.from_cli(s)
  pp! d

  puts "\nshard.yaml"
  puts(YAML.build do |yaml|
    yaml.mapping do
      d.to_yaml(yaml)
    end
  end)

  puts "\nshard.lock"
  puts(YAML.build do |yaml|
    yaml.mapping do
      d.dependency.to_yaml(yaml)
    end
  end)
end

t("github:crystal-lang/crystal-db")

t("github:crystal-lang/crystal-db#0.13.0")

which will output

d # => #<Shards::DependencyDefinition:0x1011fbbd0
 @dependency=
  #<Shards::Dependency:0x101217e60
   @name="db",
   @requirement=*,
   @resolver=
    #<Shards::GitResolver:0x10120f200
     @local_path=
      "/Users/bcardiff/.cache/shards/github.com/crystal-lang/crystal-db.git",
     @name="unknown",
     @origin_url="https://github.com/crystal-lang/crystal-db.git",
     @source="https://github.com/crystal-lang/crystal-db.git",
     @updated_cache=true>,
   @resolver_key=nil,
   @source=nil>,
 @resolver_key="github",
 @source="crystal-lang/crystal-db">

shard.yaml
---
db:
  github: crystal-lang/crystal-db

shard.lock
---
db:
  git: https://github.com/crystal-lang/crystal-db.git
d # => #<Shards::DependencyDefinition:0x1011fb240
 @dependency=
  #<Shards::Dependency:0x1012179b0
   @name="db",
   @requirement=Shards::VersionReq(@patterns=["~> 0.13.0"]),
   @resolver=
    #<Shards::GitResolver:0x10120f200
     @local_path=
      "/Users/bcardiff/.cache/shards/github.com/crystal-lang/crystal-db.git",
     @name="unknown",
     @origin_url="https://github.com/crystal-lang/crystal-db.git",
     @source="https://github.com/crystal-lang/crystal-db.git",
     @updated_cache=true>,
   @resolver_key=nil,
   @source=nil>,
 @resolver_key="github",
 @source="crystal-lang/crystal-db">

shard.yaml
---
db:
  github: crystal-lang/crystal-db
  version: 0.13.0

shard.lock
---
db:
  git: https://github.com/crystal-lang/crystal-db.git
  version: 0.13.0

[ysbaddaden]

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.

[straight-shoota]

I would go the other way: encode the full information in a single URI. This should work well. Most of the time, the natural formats are already URI-compatible. That's a fully descriptive, self-contained format that's easy to reuse in other contexts.

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. git, github, git+https, etc. go to GitResolver).

If the URI scheme does not indicate a specific resolver (e.g. https), we fall back to heuristics based on the full URI (e.g. based on known hostname, or file extension) or potentially even trial-and-error through possible resolvers (very optional).

[bcardiff]

For GitHub, from the clone menu we should support

• https://github.com/owner/repo.git
• [email protected]:owner/repo.git

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 Resolver to offer a canonical representation of the dependency for the shard.yaml. Eg: urls are downcased in the normalization and that will look odd.

I saw mentions of git+ssh somewhere. IIUC those will be [email protected]:owner/repo.git, am I missing another pattern?

Thanks for the feedback.

[bcardiff]

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?

[straight-shoota]

We could probably turn the named tuple return type from .parts_from_cli into a dedicated type which holds this information.

[bcardiff]

I think I incorporated all of the feedback.

[straight-shoota]

Following up on https://github.com/crystal-lang/shards/pull/673#issuecomment-2886164527, I propose an alternative implementation, fully based on URI parsing in #678

IMO this makes it much easier to handle things. The different routes are clearly decided by the URI scheme. All parsing is safe and sound using stdlib's URI class.

[straight-shoota]

For reference, VCS dependencies in PIP: https://pip.pypa.io/en/stable/topics/vcs-support/

[bcardiff]

The alternative implementation seems good. We can close this PR then in favor of the other. Anything else that is missing?, because it's marked as draft.

[straight-shoota]

https://github.com/crystal-lang/shards/pull/678 is a patch against the branch of this PR. We should merge that in and then continue here.

[bcardiff]

Commit pushed into this branch

[straight-shoota]

The windows test is failing because for relative URIs we expect .starts_with?("./"), .starts_with?("../") to recognize a relative path by prefix. If we want to support path's with backslash separators as well, we need to add .starts_with?(".\\"), .starts_with?("..\\")

[bcardiff]

Made some changes to use # instead of @ while supporting it also in other syntax like git@ https:// etc and not just github:

[beta-ziliani]

I gave my opinion on the high level of it. I think it's loable to center on the user, in aspects like being able to copy-paste from the browser. Thanks Branch!

[luislavena]

Folks, I think we are too much into the weeds about the pessimistic comparator versus an specific version that we are forgetting that not all shells are going to parse ~> as you expect.

Point of example, zsh:

$ cat shards
#!/usr/bin/env sh

echo "called with $@"

$ ./shards add github:foo/bar
called with add github:foo/bar

But fails with pessimistic one:

$ ./shards add github:foo/bar#~>1.2.3

$

$ cat 1.2.3
called with add github:foo/bar#~

This will force the user to quote the parameter:

$ ./shards add "github:foo/bar#~3.2.1"
called with add github:foo/bar#~3.2.1

[bcardiff]

Ok, leaving # as exact version. (Or I can drop that too).

Adding a version: manually is easy, definitely, but this whole thing is to avoid the user editing the shard.yml manually if possible. Leaving version information out seems a miss to me.

Deferring the discussion on how to support ~> 1.2.3 as a uri-sh parameter for later (maybe something along the way of #compat=1.2.3 to avoid the >.

I do want to move in to the actual code of changing the yaml based on this 😅