Skip to content

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/cli.cr
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ require "./commands/*"

module Shards
BUILTIN_COMMANDS = %w[
add
rm
build
run
check
Expand All @@ -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.
Copy link
Contributor

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.

rm [<shards>] - Remove a shard from `shard.yml`, then prune.
Copy link
Contributor

@ysbaddaden ysbaddaden May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The actual name is remove, and other dependency managers (Cargo, Bundler) just use that.

Suggested change
rm [<shards>] - Remove a shard from `shard.yml`, then prune.
remove [<shard> ...] - Remove named shard(s) from `shard.yml`.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 prune command doesn't do (it even relies on the lockfile).

init - Initialize a `shard.yml` file.
install - Install dependencies, creating or using the `shard.lock` file.
list [--tree] - List installed dependencies.
Expand Down Expand Up @@ -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"
Expand Down
92 changes: 92 additions & 0 deletions src/commands/add.cr
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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 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.

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"))
Copy link
Contributor

@ysbaddaden ysbaddaden May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to manually create the lockfile. The install command already resolves new shards.

Suggested change
Commands::Lock.new(path).run([] of String)

Commands::Install.new(path).run
end

end
end
end
41 changes: 41 additions & 0 deletions src/commands/io.cr
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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."}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: instead of failing, the add command could just invoke the init command.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 Shard::Dependency.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: that should be a Shard::Dependency object.

end
end
end
84 changes: 84 additions & 0 deletions src/commands/remove.cr
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything said above for the add command also applies to the remove command.

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