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

git-annex-remote-rclone 0.6 (new formula) #49468

Closed

Conversation

@dankessler
Copy link
Contributor

dankessler commented Jan 28, 2020

First time contributing to homebrew-core. I've tried very hard to review all of your (very well-written) guidelines, but I apologize in advance if I've overlooked anything

Add formula for git-annex-remote-rclone. This tool enables git-annex
to use any of the providers supported by rclone as a special remote
when using git-annex. This is available for debian via apt, but is not
currently in homebrew. Installation is as simple as copying a bash
script onto the path. Writing a test was tricky, since using
git-annex-remote-rclone requires creating a new special remote using
an extant rclone target, so following the guidance in the formula
cookbook, I've tried to initialize a remote using target
testtest123123, which hopefully nobody uses as a name for a remote in
rclone. If git-annex-remote-rclone is correctly installed, it will
generate a specific error message which the test checks.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@issyl0 issyl0 added the new formula label Jan 28, 2020
@jonchang

This comment has been minimized.

Copy link
Member

jonchang commented Jan 29, 2020

Installation is as simple as copying a bash
 script onto the path.

This should be bottle :unneeded then.

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Jan 29, 2020

When I fix this, is it preferred that I squash my new changes into the previous commit and force the push? Or just add a commit to the PR? I’m not clear on how strictly you want to adhere to the 1 commit : 1 formula rule from the cookbook

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Jan 29, 2020

I'm going to use this opportunity to also try to write a better test if I can. I'll make additional commits under the assumption that I can always squash them later if requested.

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 29, 2020

Eventually it'll need to be squashed before it can be merged.

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Jan 31, 2020

I've now written a substantially more robust test; details documented in commit message 594a40b. I think things are ready to go / be reviewed now.

For now I've left things in 3 separate commits, but will happily squash and do a force push on request, but I've never done a force push with a PR so am not clear what the consequences will be and am holding off until asked to do so.

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Jan 31, 2020

Well that was embarrassing; I had a second tap active which had an (audit-passing) version of the same formula, and it seems that this confuses brew audit so I missed these issues.

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Feb 4, 2020

I'm not sure what the code review process is, so I wanted to double check if there was some action I needed to take (perhaps you do want me to squash these commits?) in order to get this reviewed and (hopefully) accepted.

@chenrui333

This comment has been minimized.

Copy link
Member

chenrui333 commented Feb 4, 2020

Thanks @dankessler for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!!

@chenrui333

This comment has been minimized.

Copy link
Member

chenrui333 commented Feb 4, 2020

I'm not sure what the code review process is, so I wanted to double check if there was some action I needed to take (perhaps you do want me to squash these commits?) in order to get this reviewed and (hopefully) accepted.

I think it looks good, just one squashed commit away.

@chenrui333

This comment has been minimized.

Copy link
Member

chenrui333 commented Feb 4, 2020

Eventually it'll need to be squashed before it can be merged.

cc @dankessler

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Feb 4, 2020

@chenrui333 do you prefer I do the squashing, or do you do that when accepting the PR?

I infer from your tag that you want me to squash :)

Add formula for git-annex-remote-rclone. This tool enables git-annex
to use any of the providers supported by rclone as a special remote
when using git-annex. This is available for debian via apt, but is not
currently in homebrew. Installation is as simple as copying a bash
script onto the path. Writing a test was tricky, since using
git-annex-remote-rclone requires creating a new special remote using
an extant rclone target, so following the guidance in the formula
cookbook, I've tried to initialize a remote using target
testtest123123, which hopefully nobody uses as a name for a remote in
rclone. If git-annex-remote-rclone is correctly installed, it will
generate a specific error message which the test checks.

specify that no bottle is needed since it's a trivial install

write more robust test

We use environment variables to specify a "tmplocal" rclone remote
which is simply the local filesystem, and then test
git-annex-remote-rclone by initializing a specialremote mapped to
tmplocal. The current test goes a step further by committing contents
to the annex and then copying it to this remote. This may be more than
is necessary. Pieces of the test code involved in git-annex repo set
up, commiting, and teardown are copied verbatim from the git-annex
formula test.

Fix stylistic problems caught by audit
@chenrui333 chenrui333 force-pushed the dankessler:git-annex-remote-rclone-formula branch from f644633 to 8a37b29 Feb 4, 2020
@chenrui333

This comment has been minimized.

Copy link
Member

chenrui333 commented Feb 4, 2020

Sorry about so many back and forth label changes. 😅

@dankessler

This comment has been minimized.

Copy link
Contributor Author

dankessler commented Feb 4, 2020

No problem. So it looks like you have done the squashing, so there's nothing further you need from me, correct?

@bayandin bayandin closed this in 04fc98c Feb 5, 2020
@dankessler dankessler deleted the dankessler:git-annex-remote-rclone-formula branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.