Metacello / metacello

Metacello is a package management system for Smalltalk
MIT License
87 stars 43 forks source link

Add shortcuts for fetching GitHub repositories #537

Closed LinqLover closed 3 years ago

LinqLover commented 3 years ago

Usage:

Metacello new
    baseline: 'Metacello';
    githubUser: 'LinqLover' project: 'metacello' path: 'repository';
    get.

Open questions:

LinqLover commented 3 years ago

@dalehenrich Please see the questions above, CI fails probably because of point 2 :)

LinqLover commented 3 years ago

CI passed on my fork, still running for this repository. @dalehenrich Sorry for paging, but it would be great if you could give me a short answer on whether you would like to see additional changes on this PR (see above) or whether this can be merged! :-)

LinqLover commented 3 years ago

@dalehenrich Hey, how do you think about this PR? Should we add other convenience buttons or can I just press the Merge button? :-)

dalehenrich commented 3 years ago

sorry to have missed your earlier ping, do I read this correctly that you are changing the behavior such that the default committish is no longer 'master'? Perhaps not specifying the comittish in the url results in 'master' being defaulted elsewhere? If so then I'm comfortable with this change ... as it reduces the number of places where the default committish is hard coded ...

I have a sorta rule that behavior cannot be changed, because you never know who might depend upon that behavior and there's nothing worse than updating a project only to have things stop working ....

I think adding a test is a good idea, to "keep things honest" moving forward ...

So thumbs up and I appreciate your contributions

LinqLover commented 3 years ago

you are changing the behavior such that the default committish is no longer 'master'? Perhaps not specifying the comittish in the url results in 'master' being defaulted elsewhere?

Exactly, the decision about the default commitish is now totally up to GitHub. See also MC[Fetch]GitHubRepository>>projectZipUrlFor:versionString: and the GitHub API docs about the zipball/tarball endpoints.

I have a sorta rule that behavior cannot be changed, because you never know who might depend upon that behavior and there's nothing worse than updating a project only to have things stop working ....

I totally agree with this strategy in general, but if we took this literally in this example, it hypothetically might be possible that someone set up a GitHub repository with both a master and a main branch and configured the main branch as the default in the repository settings. In this case, if they did not explicitly specify the commitish, in the past, it would have fallen back to master but now to main ... But personally, I think this would be a too speculative apprehension, what do you think?

I think adding a test is a good idea, to "keep things honest" moving forward ...

On it

dalehenrich commented 3 years ago

I totally agree with this strategy in general, but if we took this literally in this example, it hypothetically might be possible that someone set up a GitHub repository with both a master and a main branch and configured the main branch as the default in the repository settings. In this case, if they did not explicitly specify the commitish, in the past, it would have fallen back to master but now to main ... But personally, I think this would be a too speculative apprehension, what do you think?

Haha, when the underlying system changes, you have to adapt, so I am in favor of your changes ...

LinqLover commented 3 years ago

Alright, then we should be done! New smoke tests exist and I did not find any regressions in the CI, so I'm going to merge this now. Thanks for your support!

LinqLover commented 3 years ago

you are changing the behavior such that the default committish is no longer 'master'? Perhaps not specifying the comittish in the url results in 'master' being defaulted elsewhere?

Exactly, the decision about the default commitish is now totally up to GitHub. See also MC[Fetch]GitHubRepository>>projectZipUrlFor:versionString: and the GitHub API docs about the zipball/tarball endpoints.

Argh, I need to correct myself. 'master' is still hard-coded somewhere else so currently the default branch name will still be replaced by master. This is not a regression but nevertheless not so convenient ...