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

rswift 1.1.1 (new formula) #48357

Closed
wants to merge 6 commits into from
Closed

Conversation

tomasharkema
Copy link
Contributor

Previous pr got a little bit messed-up, so i've reissued a PR with one tidy commit.

This adds the R.swift binary (from https://github.com/mac-cain13/R.swift) to brew. We got this request form multiple Cartage users.

class Rswift < Formula
desc "Get strong typed, autocompleted resources like images, fonts and segues"
homepage "https://github.com/mac-cain13/R.swift"
url "https://github.com/mac-cain13/R.swift.git", :tag => "v1.1.1", :revision => "3a6db62164d8f50ccdf43e59894fffb672fd5e3f"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap this on multiple lines:

url "...",
  :tag => "...",
  :revision => "..."

@tomasharkema
Copy link
Contributor Author

thanks for your feedback @bfontaine
i've updated the file.

@tomasharkema
Copy link
Contributor Author

@bfontaine looks like jenkins reported a false negative. what do I need to do?

@meritozh
Copy link

Tested successfully in el_capitan and mavericks. And look error message, it just install mime-types failed. So you needn't do anything

@bfontaine
Copy link
Contributor

@tomasharkema You can’t do anything about that, unfortunately.

end

test do
system "rswift", "-h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can’t we write a test that does something more substantial than e.g. -h/v? Also, use "#{bin}/rswift" instead of "rswift" to have the full path. Thanks!

Choose a reason for hiding this comment

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

Good point @bfontaine, @tomasharkema asked me to take a look at the possibilities for a better test.

I think it’s a bit hard to really use the tool in a test, since that would need a valid iOS Xcode project. Creating a fixture for that would be a bit of a hassle I think.

The project itself tests if it output is correct using CI, so if we know that the binary is functional by executing it with a -h flag then I’m pretty sure everything is ok. I agree a test that actually uses the tool is better, but I think it’s not worth the hassle and maintenance in this case.

Also, I noted some other tools also use this kind of tests, so it seems like a good fallback to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay. If it’s too hard to create a resource for this it’s fine as is. Just replace "rswift" with "#{bin}/rswift".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

url "https://github.com/mac-cain13/R.swift.git",
:tag => "v1.1.1",
:revision => "3a6db62164d8f50ccdf43e59894fffb672fd5e3f"
depends_on :xcode => "7.0"
Copy link
Member

Choose a reason for hiding this comment

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

Please leave an empty line between the :revision and first depends_on, Thanks!

@tomasharkema
Copy link
Contributor Author

@DomT4 Thanks for your comment. I've fixed it.

I took the liberty to also adjust the audit script in such a way that your previous comments on my code are checked for by the audit command in the future.

@@ -647,6 +647,10 @@ def audit_text
Please add ENV.prepend_path \"PATH\", \"#{need_npm}"\ to def install
EOS
end

if text =~ /=>.*[^,\n]\n[^\n].*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This would match quite a lot of legitimate stuff, e.g.:

def install
  bin.install "foo" => "bar"
end

@bfontaine
Copy link
Contributor

@tomasharkema Thank you for taking the time to improve the audit rules. However you might want to do that in a separate PR so that this one can be focused on rswift and get merged faster.

Tomas Harkema added 2 commits February 10, 2016 13:28
…revent this issues in the future :)"

This reverts commit bcc0939.
@tomasharkema
Copy link
Contributor Author

@bfontaine you're totally right. i've reverted the changes on audit.rb.

@tomasharkema
Copy link
Contributor Author

@bfontaine do you agree with @mac-cain13 's remark about the test?

#48357 (comment)

@bfontaine
Copy link
Contributor

Thank you @tomasharkema for your contribution to Homebrew; we appreciate it! 🎉

@bfontaine bfontaine closed this in 3e849b5 Feb 11, 2016
flier pushed a commit to flier/homebrew that referenced this pull request Feb 17, 2016
Closes Homebrew#48357.

Signed-off-by: Baptiste Fontaine <batifon@yahoo.fr>
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants