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 abbreviated owner/repo Git URL format #242

Merged
merged 8 commits into from Sep 12, 2020

Conversation

9999years
Copy link
Contributor

The GitHub hub tool allows using owner/repo as an abbreviated form of https://github.com/owner/repo.git. This change supports that format in the --git command-line argument.

It also improves the command-line argument documentation.

The GitHub [hub] tool allows using `owner/repo` as an abbreviated form
of `https://github.com/owner/repo.git`. This change supports that format
with the `--git` command-line argument.

It also improves the command-line argument documentation.

[hub]: https://github.com/github/hub
Backslashes strike again
Copy link
Collaborator

@k0pernicus k0pernicus left a comment

Choose a reason for hiding this comment

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

Thanks for proposing the changes :)

However, this will need another review.

@DD5HT, @ashleygwilliams ?

/// [hub].
///
/// [hub]: https://github.com/github/hub
pub fn new_abbr(git: &str, branch: String) -> Result<Self, failure::Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little embarrassed with this as we definitely become more and more closer to github and not providers like gitlab, atlassian products, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's... unfortunate. Both because of GitHub's awful politics (drop ICE, you bastards) and because we've effectively centralized a purposefully distributed VCS. (Not like Atlassian's any good either, though.)

But also... it's a very convenient feature that helps improve usability. It might be nice to have a (hardcoded...?) name-to-URL map as well, for the projects in TEMPLATES.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And, just pragmatically, every single template currently listed in TEMPLATES.md is on GitHub anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And, just pragmatically, every single template currently listed in TEMPLATES.md is on GitHub anyways.

Yep but, in my own opinion, this does not have to push us to "close" some doors...

Let's wait if other maintainers have an opinion on that or not (if they come).

Copy link
Member

Choose a reason for hiding this comment

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

Just a thought on this, right now the --git cli param defaults to github, which is fine. What about introducing others like --gitlab or --bitbucket that behave as --git just with a different url string that is used here in line 57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would support that, but remember that the good is the enemy of the perfect and this code is here, working and tested, now. Of course I'll defer to the maintainers but I don't know if this is worth bikeshedding that much over.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's an improvement anyway, means the old style with a full repo URL is still available.

So I would vote for merge as it is and wait for feedback / wishes from users after releasing it.

Copy link
Member

Choose a reason for hiding this comment

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

Hey there, can we just land this? I don't see any reason to wait any longer.
Also we should plan a new release.. I guess it's time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As it seems the original maintainers don't care about the project anymore, I think we can bypass their approval indeed.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I would not call it baypassing, if something goes in a direction they don't want, then they surely would jump in and make decissions, not seeing them making any statements for means just they agree with everything. 😁

@k0pernicus
Copy link
Collaborator

k0pernicus commented Sep 12, 2020

I resolved the current conflicts with master.
I am waiting for the tests to run, before merging to master.

@k0pernicus k0pernicus merged commit d2bb2e5 into cargo-generate:master Sep 12, 2020
@k0pernicus
Copy link
Collaborator

Done, thanks again for your work :)

@k0pernicus k0pernicus added this to the 0.5.1 milestone Sep 14, 2020
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.

None yet

3 participants