-
-
Notifications
You must be signed in to change notification settings - Fork 105
Add remove and add commands #672
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,6 +3,8 @@ require "./commands/*" | |||||
|
||||||
module Shards | ||||||
BUILTIN_COMMANDS = %w[ | ||||||
add | ||||||
rm | ||||||
build | ||||||
run | ||||||
check | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The actual name is
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
init - Initialize a `shard.yml` file. | ||||||
install - Install dependencies, creating or using the `shard.lock` file. | ||||||
list [--tree] - List installed dependencies. | ||||||
|
@@ -90,6 +94,20 @@ module Shards | |||||
Commands::Run.run(path, targets, run_options, options) | ||||||
when "check" | ||||||
Commands::Check.run(path) | ||||||
when "add" | ||||||
urls = args[1..-1] | ||||||
version = nil | ||||||
if urls.size > 1 && !urls.last.includes?("://") && !urls.last.includes?('@') | ||||||
version = urls.pop | ||||||
end | ||||||
Commands::Add.new(path).run(urls, version) | ||||||
when "rm" | ||||||
url = args[1] | ||||||
if url.nil? | ||||||
Log.error{"ERROR: missing dependency URL"} | ||||||
exit 1 | ||||||
end | ||||||
Commands::Remove.new(path).run(url) | ||||||
when "init" | ||||||
Commands::Init.run(path) | ||||||
when "install" | ||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,92 @@ | ||||
require "./command" | ||||
require "./io" | ||||
|
||||
module Shards | ||||
module Commands | ||||
class Add < Command | ||||
def run(urls : Array(String), version : String? = nil) | ||||
spec_path = File.join(path, SPEC_FILENAME) | ||||
raise Error.new("#{SPEC_FILENAME} not found") unless File.exists?(spec_path) | ||||
|
||||
urls.each do |url| | ||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 We could even detect the usage of the |
||||
dependencies_index = -1 | ||||
dependencies_indentation = "" | ||||
|
||||
lines.each_with_index do |line, index| | ||||
if line =~ /^(\s*)dependencies\s*:/ | ||||
dependencies_index = index | ||||
dependencies_indentation = $1 | ||||
break | ||||
end | ||||
end | ||||
|
||||
if dependencies_index == -1 | ||||
lines << "" if lines.last != "" | ||||
lines << "dependencies:" | ||||
dependencies_index = lines.size - 1 | ||||
dependencies_indentation = "" | ||||
end | ||||
|
||||
dep_name = dep[:name] | ||||
dep_start_index = -1 | ||||
dep_end_index = -1 | ||||
|
||||
(dependencies_index + 1).upto(lines.size - 1) do |i| | ||||
break if i >= lines.size || lines[i] =~ /^\S/ && !lines[i].starts_with?("#") | ||||
if lines[i] =~ /^\s+#{Regex.escape(dep_name)}\s*:/ | ||||
dep_start_index = i | ||||
|
||||
j = i + 1 | ||||
while j < lines.size && (lines[j].empty? || lines[j].starts_with?("#") || lines[j] =~ /^\s+/) | ||||
if lines[j] =~ /^(\s+)/ && $1.size > lines[i].index(/\S/).not_nil! | ||||
dep_end_index = j | ||||
end | ||||
j += 1 | ||||
end | ||||
|
||||
break | ||||
end | ||||
end | ||||
|
||||
dep_indentation = "#{dependencies_indentation} " | ||||
prop_indentation = "#{dependencies_indentation} " | ||||
dep_lines = ["#{dep_indentation}#{dep_name}:"] | ||||
|
||||
dep_lines << "#{prop_indentation}#{dep[:provider]}: #{dep[:repo]}" | ||||
dep_lines << "#{prop_indentation}version: #{version}" if version | ||||
|
||||
if dep_start_index != -1 | ||||
lines.delete_at(dep_start_index..dep_end_index) | ||||
dep_lines.each_with_index do |line, idx| | ||||
lines.insert(dep_start_index + idx, line) | ||||
end | ||||
else | ||||
insert_index = dependencies_index + 1 | ||||
|
||||
while insert_index < lines.size && | ||||
(lines[insert_index].empty? || | ||||
lines[insert_index].starts_with?("#") || | ||||
lines[insert_index] =~ /^\s+/) | ||||
insert_index += 1 | ||||
end | ||||
|
||||
dep_lines.each_with_index do |line, idx| | ||||
lines.insert(insert_index + idx, line) | ||||
end | ||||
end | ||||
|
||||
File.write(spec_path, lines.join("\n")) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: If the dependencies: [] dependencies:
one: { git: "..." }
two: { git: "..." } Suggestion: follow @straight-shoota's idea to parse the original |
||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to manually create the lockfile. The
Suggested change
|
||||
Commands::Install.new(path).run | ||||
end | ||||
|
||||
end | ||||
end | ||||
end |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: this file ain't a command but a set of helpers. They must be put somewhere else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You said a lot of stuff, but I am curious about this, where "somewhere else" should they be put? Ideally helpers shouldnt exist at all, and the idea was that any of the core team take this code and edit it until they're happy with it, since yeah, obviously shards isn't going to use only github gitlab or codeberg. This was ripped from my tool which restricts to those because it shows shard stats, and not every git provider has an API to get that from (i.e. stars, contributors). Regardless, its open for editing, as I don't have time for this at the moment. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
module Shards | ||
module Commands | ||
def self.read_shard_yml : Array(String) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: instead of failing, the |
||
exit 1 | ||
end | ||
end | ||
|
||
def self.write_shard_yml(lines : Array(String)) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: As stated above, Shards supports much more resolvers than Git. Each resolver should be in charge to transform an input String into a |
||
hosts = { | ||
"github.com" => "github", | ||
"gitlab.com" => "gitlab", | ||
"codeberg.org" => "codeberg" | ||
} | ||
|
||
uri = URI.parse(url) | ||
provider = hosts[uri.host]? | ||
parts = uri.path.split("/").reject(&.empty?) | ||
|
||
if parts.size < 2 | ||
raise Error.new("Invalid git URL format") | ||
end | ||
if !provider | ||
provider = "github" | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Shards accept whatever Git repository, it doesn't even have to be a remote repository (it can be a local path) and certainly not be restricted to a fixed set of 3 services. We can beautify for the few known servers as per the SPEC, but we must accept anything. For example: shards' specs use local git paths. |
||
|
||
return { | ||
name: parts.last.gsub(".git", "").downcase, | ||
repo: "#{parts[0]}/#{parts[1]}", | ||
provider: provider | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: that should be a |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
require "./command" | ||
require "./io" | ||
|
||
module Shards | ||
module Commands | ||
class Remove < Command | ||
def run(url : String) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Everything said above for the |
||
dep = Commands.git_url_to_dependency(url) | ||
Log.info{"Removing dependency: #{dep[:name]}"} | ||
|
||
lines = Commands.read_shard_yml | ||
dependencies_index = -1 | ||
|
||
lines.each_with_index do |line, index| | ||
if line =~ /^(\s*)dependencies\s*:/ | ||
dependencies_index = index | ||
break | ||
end | ||
end | ||
|
||
if dependencies_index == -1 | ||
Log.warn{"Dependency #{dep[:name]} not found, nothing to remove."} | ||
return | ||
end | ||
|
||
dep_name = dep[:name] | ||
dep_start_index = -1 | ||
dep_end_index = -1 | ||
dep_indentation = nil | ||
|
||
(dependencies_index + 1).upto(lines.size - 1) do |i| | ||
break if i >= lines.size || (lines[i] =~ /^\S/ && !lines[i].starts_with?("#")) | ||
|
||
if lines[i] =~ /^(\s+)#{Regex.escape(dep_name)}\s*:/ | ||
dep_start_index = i | ||
dep_indentation = $1.size | ||
|
||
j = i + 1 | ||
while j < lines.size | ||
if !lines[j].empty? && !lines[j].starts_with?("#") && lines[j] =~ /^(\s*)\S/ | ||
current_indent = $1.size | ||
if current_indent <= dep_indentation | ||
break | ||
end | ||
dep_end_index = j | ||
end | ||
j += 1 | ||
end | ||
|
||
break | ||
end | ||
end | ||
|
||
if dep_start_index != -1 | ||
if dep_end_index != -1 | ||
lines.delete_at(dep_start_index..dep_end_index) | ||
else | ||
lines.delete_at(dep_start_index) | ||
end | ||
|
||
has_other_deps = false | ||
(dependencies_index + 1).upto(lines.size - 1) do |i| | ||
break if i >= lines.size || (lines[i] =~ /^\S/ && !lines[i].starts_with?("#")) | ||
if lines[i] =~ /^\s+\S+\s*:/ | ||
has_other_deps = true | ||
break | ||
end | ||
end | ||
|
||
if !has_other_deps | ||
lines.delete_at(dependencies_index) | ||
end | ||
|
||
Commands.write_shard_yml(lines) | ||
Commands::Prune.new(path).run | ||
|
||
Log.info{"Removed dependency #{dep[:name]}."} | ||
else | ||
Log.warn{"Dependency: #{dep[:name]} not found, nothing to remove."} | ||
end | ||
end | ||
end | ||
end | ||
end |
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: 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.