[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Try cloning repo from main & master #10843

Merged
merged 8 commits into from
Jun 28, 2022
Merged

Try cloning repo from main & master #10843

merged 8 commits into from
Jun 28, 2022

Conversation

Zackere
Copy link
Contributor
@Zackere Zackere commented May 23, 2022

Resolve #10458

Description

Types of change

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@svlandeg svlandeg added enhancement Feature requests and improvements feat / cli Feature: Command-line interface labels May 23, 2022
@adrianeboyd
Copy link
Contributor

Sorry for the delayed reply! We need to do some internal testing make sure this doesn't break any existing workflows (it's not covered well by the test suite), but we definitely think this is a good idea and thanks for the PR!

@pmbaumgartner
Copy link
Contributor
pmbaumgartner commented Jun 23, 2022 •
edited
Loading

@Zackere Thank you for this PR! Since we were previously assuming a single default, and now we have a list of defaults to check (main and master), I added a utility function to first see if a repo:branch exists, rather than attempting to go straight to a clone and relying on the failure of a clone to indicate a pull wasn't available. I also added some more distinct failure messages if a branch isn't found for a repo.

I tested this on an example repo I made from an existing project (https://github.com/pmbaumgartner/clone-testing), as well as the current explosion/projects repo. Here are the following outputs:

A branch that doesn't exist:

python -m spacy project clone template --repo https://github.com/pmbaumgartner/clone-testing --branch NOPE

✘ repo: https://github.com/pmbaumgartner/clone-testing (branch: NOPE)
does not exist.

When I have it set to main as the default branch:

python -m spacy project clone template --repo https://github.com/pmbaumgartner/clone-testing 

✔ Cloned 'template' from 'pmbaumgartner/clone-testing' (branch
'main')
/Users/peter/projects/spacy-explosion/template
✔ Your project is now ready!
To fetch the assets, run:
python -m spacy project assets /Users/peter/projects/spacy-explosion/template

In the rare case that neither main or master exist (I renamed the main branch to alt):

python -m spacy project clone template --repo https://github.com/pmbaumgartner/clone-testing 

✘ No branch provided and attempted default branches 'main', 'master'
do not exist.

And finally, a test that the default of using the projects repo still works (now w/ additional info about branch used):

 python -m spacy project clone experimental/ner_spancat                   
               
✔ Cloned 'experimental/ner_spancat' from 'explosion/projects' (branch
'v3')
/Users/peter/projects/spacy-explosion/ner_spancat
✔ Your project is now ready!
To fetch the assets, run:
python -m spacy project assets /Users/peter/projects/spacy-explosion/ner_spancat

svlandeg
svlandeg reviewed Jun 28, 2022
View reviewed changes
Copy link
Member
@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Thanks again @Zackere for the PR and to @pmbaumgartner for the extensive tests & utility function!

I had just one small comment and then I think this should be good to merge.

spacy/cli/project/clone.py Outdated Show resolved Hide resolved
@pmbaumgartner @svlandeg
Update spacy/cli/project/clone.py
7702652 
Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
@pmbaumgartner pmbaumgartner merged commit 8ffff18 into explosion:master Jun 28, 2022
polm pushed a commit that referenced this pull request Jul 11, 2022
@Zackere @pmbaumgartner
@svlandeg @polm
Try cloning repo from main & master (#10843)
4570456 
* Try cloning repo from main & master

* fixup! Try cloning repo from main & master

* fixup! fixup! Try cloning repo from main & master

* refactor clone and check for repo:branch existence

* spacing fix

* make mypy happy

* type util function

* Update spacy/cli/project/clone.py

Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>

Co-authored-by: Peter Baumgartner <5107405+pmbaumgartner@users.noreply.github.com>
Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svlandeg svlandeg svlandeg left review comments

Assignees
No one assigned
Labels
enhancement Feature requests and improvements feat / cli Feature: Command-line interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spacy project clone assumes master not main.
4 participants
@Zackere @adrianeboyd @pmbaumgartner @svlandeg