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

Add shortcuts for fetching GitHub repositories #537

Merged
merged 9 commits into from
Feb 16, 2021

Conversation

LinqLover
Copy link
Collaborator

@LinqLover LinqLover commented Nov 7, 2020

Usage:

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

Open questions:

  • Should we maybe also default path to 'repository' or isn't this convention?
  • How can I fill in the id tag in the monticello.meta/version file below manually?
  • Should I add another test for the new convenience method?

@LinqLover
Copy link
Collaborator Author

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

@LinqLover
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@dalehenrich
Copy link
Member

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
Copy link
Collaborator Author

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

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
Copy link
Collaborator Author

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 LinqLover merged commit a2336b0 into Metacello:master Feb 16, 2021
@LinqLover LinqLover deleted the feat/github-shortcuts branch February 16, 2021 14:47
LinqLover added a commit to LinqLover/ColorContextMenu that referenced this pull request Feb 25, 2021
- 🎨 Beautiful screenshots
- ⚡ Use new convenient selector for Metacello (Metacello/metacello#537)
@LinqLover
Copy link
Collaborator Author

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

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