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

Cleanup and missing tests #137

Merged
merged 7 commits into from
Jun 30, 2016
Merged

Cleanup and missing tests #137

merged 7 commits into from
Jun 30, 2016

Conversation

mptre
Copy link
Owner

@mptre mptre commented Jun 19, 2016

Minor changes this time. Let me know what you think.

- Don't use puts for non compile-time constants

- Turn output_description into a local variable
Turn '\n' literals in the output field into proper new lines in order to
match the output when the `-o` option is present.
While at it, break long lines.
@calleluks
Copy link
Collaborator

Sorry for not getting back to you earlier @mptre, I've been on vacation. I think these changes look great and will merge them!

@calleluks calleluks merged commit 719087f into mptre:master Jun 30, 2016
@mptre
Copy link
Owner Author

mptre commented Jun 30, 2016

No worries, thanks for merging!

@mptre mptre deleted the minor branch June 30, 2016 09:48
@calleluks
Copy link
Collaborator

@mptre On that topic: I think the work you're doing on Pick is great and I really appreciate your code contributions as well as your insights on mine and others' PRs.

Would you like to be added as a contributor on this repo with commit access? I think that could streamline our workflow quite a bit by reducing the number of cross-repo PRs in addition to removing me as a bottle-neck for merging PRs while I'm traveling.

@mptre
Copy link
Owner Author

mptre commented Jul 2, 2016

Would you like to be added as a contributor on this repo with commit access? I think that could streamline our workflow quite a bit by reducing the number of cross-repo PRs in addition to removing me as a bottle-neck for merging PRs while I'm traveling.

I'd be honored! I don't think the time taken between creation and
merging of a PR is an issue, even when you're on vacation. But not
having to run my own fork with branches would make the process less
tedious. However, I do like our current PR-workflow since every thing
gets reviewed. I guess we would continue with the current workflow but I
would be able to create branches in the pick-repository directly
instead. This also seems compliant with thoughtbot's workflow.

@calleluks
Copy link
Collaborator

@mptre Great! I've sent an invitation.

I agree that all code that goes into master should go through a PR. I do my best to follow this workflow: https://github.com/thoughtbot/guides/tree/83cfbe7327d574efc1a8943c9e1f60b9af751e1b/protocol/git

I guess we would continue with the current workflow but I would be able to create branches in the pick-repository directly instead.

Sounds good! After getting a "LGTM" or similar from someone else you'll also be able to rebase and merge your own branches which gets rid of a lot of the "could you please rebase again before I merge" dance.

If you have any questions, don't hesitate to ask me or anyone else; we're happy to answer them!

@mptre
Copy link
Owner Author

mptre commented Jul 6, 2016

Great! May I add myself as an author to the README and man-page?

@calleluks
Copy link
Collaborator

Please do! Sorry for not doing it myself earlier.

@mptre mptre mentioned this pull request Jul 7, 2016
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