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

tsrc sync -r support #362

Closed
wants to merge 14 commits into from
Closed

tsrc sync -r support #362

wants to merge 14 commits into from

Conversation

vonpupp
Copy link
Contributor

@vonpupp vonpupp commented Jul 6, 2022

Hello,

Following up on #359, I took the liberty of submit this PR to add -r support to the sync command.

I had to take decisions because of the -r arg collision, so I decided to change the regex inclusive arg to -i and the exclusive arg to -e, which made sense to me.

I ran lint and tests. Everything seems to be okay. I also fixed some typos in the doc regarding multiple remotes.

I hope this makes sense and gets accepted. Best regards.

@vonpupp
Copy link
Contributor Author

vonpupp commented Jul 6, 2022

I forgot to mention. I didn't update the change log file, since it is not included in contrib/dev.md guide and I am not sure how the releases are handled nor if the PR is going to be accepted.

@gdubicki
Copy link
Member

I resolved the conflicts and did a small refactor that I felt was necessary: after the cli arg rename from -i to -e the iregex name became inconsistent and misleading. Please see 2c749a1 to check what I did with it.

I think that to merge this PR we just need to resolve that single review comment. Can you do that please, @vonpupp?

It would be great if you could also add a test for the new cli arg for tsrc sync, but I think that because we kept you waiting for so long here, to follow the essence of the optimistic merging, we can accept not adding this test. Just let us know what you choose!

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.

3 participants