Skip to content
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

Support fully qualified git clone uris for 'basher install' #70

Closed
wants to merge 5 commits into from

Conversation

lordvlad
Copy link

First shot at closing #67

@juanibiapina
Copy link
Member

Overall I like the direction this is going. Here's a few thoughts that I could only after looking at your PR:

The check */*/* doesn't seem to be the best anymore because it's including at least two cases and it's becoming a little difficult to follow. It would be cool to list in the documentation all accepted patterns for specifying packages, then refactor the logic to better account for them. What I can come up with from the top of my head:

  • Fully qualified

    • https://github.com/basherpm/basher.git (-ssh flag can't be present)
    • ssh://[email protected]/basherpm/basher.git (-ssh flag can't be present)
  • Git url

  • Shortcuts

    • basherpm/basher with or without --ssh
    • github.com/basherpm/basher with or without --ssh

It also seems that multiple level packages are only supported when specifying fully qualified URLs (which might be a good restriction), or we could change the second shortcut to be github.com:basherpm/basher, which kind of resembles the git url format. I think having support for multiple level only when using full URLs is better for now because it's simpler and we could still make other changes in the future.

basher install is missing new tests.

Besides all that, you also need to make sure you're able to run any commands which take a package name only (update, uninstall etc) and add the corresponding tests. Updating the documentation describing how to specify a package is also important in the README.

@lordvlad
Copy link
Author

lordvlad commented May 9, 2019

Thanks for the review, @juanibiapina. I'll see if I find some time this weekend to improve on the PR

@juanibiapina
Copy link
Member

closing due to inactivity.

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