Skip to content

Add/remove command #192

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

Closed
wants to merge 1 commit into from
Closed

Add/remove command #192

wants to merge 1 commit into from

Conversation

nedpals
Copy link

@nedpals nedpals commented Feb 18, 2018

Keep in mind that this is still a work in progress. I'm still figuring out how will this plays well with the existing features/things found on Shards and integrate them seamlessly.

The much awaited feature for Shards is here (from #144) and is based on the work I've done on Sharn. It allows you to add or remove dependencies without manually editing the shards.yml file and installs them automatically.

Same as Sharn, the pattern is still the same.

shards add author/repo#[email protected]
shards add gitlab:author/repo#[email protected]

Roadmap

  • Add command
    • Path/repo extraction
    • Merge new deps into the spec
    • Write the merged dependencies set into the spec
    • Add dev dependencies with the --dev flag
  • Remove command
    • Extract spec deps and remove selected
    • Write it back into the spec file.

Also, I haven't tried using unit tests yet and still confuses me how it works.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Sigh. I wish nobody would have taken the time to start implementing these. I strongly believe these commands are totally useless. Expect me to be very-very nit-picky while reviewing!

  1. Let's avoid the remove command. The add command is already enough to respond to the initial demand: quickly add a dependency.

  2. Let's avoid specifying a version or branch or whatever. We quickly add a dependency from a browser copied URI:

    $ shards add https://github.com/ysbaddaden/earl

  3. You can't assume repo in github.com/user/repo to be the dependency name, since lots of repository have a .cr suffix, crystal- prefix or -crystal suffix in their name. The only proper solution is to try to download and parse the SPEC_FILENAME from the remote repository and extract it's name (and of course raise if SPEC_FILENAME can't be found).

Also, some quick observations:

  1. don't use abbreviations and beware of singular/plural usages:
    • --dev => --development,
    • repo => repository,
    • deps => dependency or dependencies;
  2. never call puts, use Shards.logger to log anything (don't hesitate to print useful debug/info) and raise on errors (e.g. dependency already defined);
  3. avoid committing unrelated format-changes (e.g. removed lines or indented commented out blocks).

@@ -30,6 +30,7 @@ module Shards
opts.on("--no-color", "") { self.colors = false }
opts.on("--version", "") { puts self.version_string; exit }
opts.on("--production", "") { self.production = true }
opts.on("--dev", "") { self.dev = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

No abbreviations please, this should be --development.

I'm uncomfortable having both --production (don't install development dependencies) and --development (add development dependency to shard.yml as global options, but for the time being individual commands didn't need special flags, yet. Maybe an evolution should allow this (later).

@ysbaddaden
Copy link
Contributor

@plainas Wrong thread. Keep it focused on reviewing this pull request (and delete/paste your comment somewhere else, thanks).

@nedpals
Copy link
Author

nedpals commented Apr 3, 2018

I have stopped the development and will close this PR. Seems that I have lost interest in making this feature for an obvious reason. Don't want to make a discussion but it is not only you would be using this. It would be a key feature for people who are new to Crystal and used to npm-esque type of package managers and wouldn't want to make their shards.yml be unreadable by the program itself. Asterite and others have agreed with this approach (for now) because it focuses more on speed and time to type because it is a CLI. It doesn't matter if its very expensive to maintain. That is why there is GitHub and people can contribute which in return would gradually be stable.

I apologize to anyone who were hyped in this feature. This is my first major PR for a large project and would like to be part of Crystal's ecosystem evolution. The code review does help me a lot but its my first time and so there were no sort-of definite "style guides" for me to be guided on how to contribute and how the program's code base works. On the other hand, I don't want to disappoint anyone that I made this commit and the design decisions I made for it to work on shards. I know it is highly discouraged to download a separate helper but in the meantime I would continue my developments in Sharn in order for me to make it more stable and as much as possible reduce the problems it such as YAML formatting so that in the future implementation and user experience would be great for all when it arrives on shards.

Edit: Just added some new information and fixed some punctuations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants