Skip to content
This repository has been archived by the owner on May 17, 2023. It is now read-only.

Several improvements #6

Merged
merged 11 commits into from
Jan 8, 2014
Merged

Several improvements #6

merged 11 commits into from
Jan 8, 2014

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Dec 27, 2013

see the commits for what exactly has been changed. the public API should be the same as before, but will skip a few unnecessary commands.

@dbravender
Copy link
Owner

@Turbo87 Sorry I haven't had time to check this yet. I do appreciate the pull request. I'll test this in the next few days. Happy New Year!

@Turbo87
Copy link
Contributor Author

Turbo87 commented Jan 1, 2014

don't worry, I'm on skiing vacation at the moment. take your time ;)

warn_only=True, quiet=True).succeeded:
return True


Copy link
Owner

Choose a reason for hiding this comment

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

This might not be necessary since git will do the right thing if the remote commit that is being pushed is already in the remote immutable object store.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right about that, but my aim was to keep the console output clean. that's basically why I check if an action is necessary before performing it.

Copy link
Owner

Choose a reason for hiding this comment

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

👍 let's keep the change.

@dbravender
Copy link
Owner

Overall I think think these changes are great. I do have a few nitpicks that I mentioned above. I think we need to add a test suite for this tool so we don't need to manually test changes. I opened #7 for this.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Jan 6, 2014

A test suite would be nice. I'm not entirely sure how to test something like this, but I guess it won't be entirely impossible ;)

Do you want to have the test suite before merging this or is that independent?

@dbravender
Copy link
Owner

Do you want to have the test suite before merging this or is that independent?

Totally independent.

dbravender added a commit that referenced this pull request Jan 8, 2014
@dbravender dbravender merged commit 969bb34 into dbravender:master Jan 8, 2014
@Turbo87
Copy link
Contributor Author

Turbo87 commented Jan 8, 2014

Gracias :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants