Add remove and add commands #672
Conversation
|
This pull request has been mentioned on Crystal Forum. There might be relevant details there: |
There was a problem hiding this comment.
Praise: Thanks for starting something on this topic. It's pretty nice to keep the comments and overall the file format. Luckily yaml doesn't care much about indentation so that shouldn't break too much.
As expected, this is much more complex than it ought to be. I left a bunch of comments because shards supports much more than github, and yaml is a loose format.
Oh, and there should be specs, especially for all the edge cases.
| @@ -23,6 +25,8 @@ module Shards | |||
| Commands: | |||
| build [<targets>] [<build_options>] - Build the specified <targets> in `bin` path, all build_options are delegated to `crystal build`. | |||
| check - Verify all dependencies are installed. | |||
| add [<url>] [<version>] - Add a shard to `shard.yml`, then install it. | |||
| rm [<shards>] - Remove a shard from `shard.yml`, then prune. | |||
There was a problem hiding this comment.
Suggestion: The actual name is remove, and other dependency managers (Cargo, Bundler) just use that.
| rm [<shards>] - Remove a shard from `shard.yml`, then prune. | |
| remove [<shard> ...] - Remove named shard(s) from `shard.yml`. |
There was a problem hiding this comment.
Thought: Automatically pruning will remove unrelated installed dependencies. Neither Cargo nor Bundler seem to uninstall anything (they only modify the dep file). We might want to regenerate the lockfile, though, which the prune command doesn't do (it even relies on the lockfile).
There was a problem hiding this comment.
Issue: this file ain't a command but a set of helpers. They must be put somewhere else.
| @@ -23,6 +25,8 @@ module Shards | |||
| Commands: | |||
| build [<targets>] [<build_options>] - Build the specified <targets> in `bin` path, all build_options are delegated to `crystal build`. | |||
| check - Verify all dependencies are installed. | |||
| add [<url>] [<version>] - Add a shard to `shard.yml`, then install it. | |||
There was a problem hiding this comment.
Thought: The add command must support all the resolvers supported by Shards, that is PATH, GIT, FOSSIL and MERCURIAL. We can detect the common hosts from the URL, and thus the resolver, but we need some explicit args for the rest. The remote URL can also be a local path:
<path>=> assumes--path<url>=> assumes from known hosts, and fails otherwise;--path <path>--git <path_or_url>[@<version]--fossil <path_or_url>[@<version]--mercurial <path_or_url>[@<version](and/or--hg?)
We should also be able to target the development dependencies:
--dev[elopment]
And maybe:
--branch <branch>--tag <tag>--commit <commit>
And fail if we try to use these with the PATH resolver.
| end | ||
| end | ||
|
|
||
| File.write(spec_path, lines.join("\n")) |
There was a problem hiding this comment.
Issue: If the shard.yml uses [] or {} notations, this will generate invalid code. It's valid YAML and we never restricted their use. We don't have to support them, but we musn't generate an invalid YAML file. For example:
dependencies: []dependencies:
one: { git: "..." }
two: { git: "..." }Suggestion: follow @straight-shoota's idea to parse the original shard.yml and add the dependency to it, then parse the patched shard.yml and compare whether the dependencies match. That would valid that the file is a valid YAML file and a valid SPEC file, and that all the dependencies are correctly defined (we didn't break anything).
| Log.info { "Added dependency #{dep[:name]} from #{dep[:provider]}: #{dep[:repo]}#{version ? " with version #{version}" : ""}" } | ||
| end | ||
|
|
||
| Commands::Lock.new(path).run([] of String) |
There was a problem hiding this comment.
No need to manually create the lockfile. The install command already resolves new shards.
| Commands::Lock.new(path).run([] of String) |
| dep = Commands.git_url_to_dependency(url) | ||
| Log.info { "Adding dependency: #{dep[:name]} from #{dep[:provider]}: #{dep[:repo]}" } | ||
|
|
||
| lines = File.read_lines(spec_path) |
There was a problem hiding this comment.
Suggestion: The command should focus on the logic (parse, add dependency, validate, file, invoke install), not go into the details of parsing and manipulating the YAML file. There should be a custom parser to parse and patch the YAML file (and specs to validate its behavior).
The manual parsing is interesting, but maybe we could follow @straight-shoota's idea to parse the file using libyaml to detect the dependencies mapping position (start/end position and indent) and of each dependency, using the official parser (more robust), then open the file and apply patches at the discovered positions. I feel like it would be end up being simpler.
We could even detect the usage of the [] or {} notations, and either fail early, or support some simple cases.
| name: parts.last.gsub(".git", "").downcase, | ||
| repo: "#{parts[0]}/#{parts[1]}", | ||
| provider: provider | ||
| } |
There was a problem hiding this comment.
Suggestion: that should be a Shard::Dependency object.
| File.write("shard.yml", lines.join("\n")) | ||
| end | ||
|
|
||
| def self.git_url_to_dependency(url : String) : NamedTuple(name: String, repo: String, provider: String) |
There was a problem hiding this comment.
Suggestion: As stated above, Shards supports much more resolvers than Git. Each resolver should be in charge to transform an input String into a Shard::Dependency.
| begin | ||
| File.read_lines("shard.yml") | ||
| rescue | ||
| Log.error{"No shard.yml was found in the current directory."} |
There was a problem hiding this comment.
Suggestion: instead of failing, the add command could just invoke the init command.
| module Shards | ||
| module Commands | ||
| class Remove < Command | ||
| def run(url : String) |
There was a problem hiding this comment.
Everything said above for the add command also applies to the remove command.
As discussed, edits can be made to match exact requirements for checks.