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 pod repo remove [name] command #1493

Merged
merged 3 commits into from
Oct 21, 2013
Merged

Add pod repo remove [name] command #1493

merged 3 commits into from
Oct 21, 2013

Conversation

joshkalpin
Copy link
Member

Deletes the repo named in the command. As requested by @orta in #1484. Not the best at ruby, but I think this should do the trick.

Deletes the repo named in the command.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 05698a8 on Kapin:master into 29697f4 on CocoaPods:master.

@orta
Copy link
Member

orta commented Oct 19, 2013

Wow! Ace. Nice first PR, good spot that git use remove for their command to remove repos.

@@ -16,7 +16,7 @@ module Pod
Command.parse(%w{ setup }).should.be.instance_of Command::Setup
Command.parse(%w{ spec create }).should.be.instance_of Command::Spec::Create
Command.parse(%w{ spec lint }).should.be.instance_of Command::Spec::Lint
Command.parse(%w{ repo update }).should.be.instance_of Command::Repo::Update
Copy link
Member

Choose a reason for hiding this comment

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

I don't think tha removing this line was intentional :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to ask about that. There were two asserts for repo update that were identical. I can add this one back in but it seemed unusual to verify that the parser works for the same command twice. However, I'll definitely change it if there was a reason for it :)

Copy link
Member

Choose a reason for hiding this comment

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

Indeed you're right 👍. However it might make sense to keep them grouped by command. I.e. move this line up with the other repo checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 91d0294 on Kapin:master into 29697f4 on CocoaPods:master.

@fabiopelosin
Copy link
Member

Looks great, do you mind adding a note to the changelog crediting yourself?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5e5e96f on Kapin:master into 29697f4 on CocoaPods:master.

@fabiopelosin
Copy link
Member

👍 Thanks for the contribution!

fabiopelosin added a commit that referenced this pull request Oct 21, 2013
Add pod repo remove [name] command
@fabiopelosin fabiopelosin merged commit f7766cc into CocoaPods:master Oct 21, 2013
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.

4 participants