Skip to content

add available languages to cask info command #3220

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.

Sign up for GitHub

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

Jump to bottom
Merged
reitermarkus merged 2 commits into from
Oct 3, 2017
Merged

add available languages to cask info command #3220

reitermarkus merged 2 commits into from
Oct 3, 2017

Conversation

@yzguy
Copy link
Contributor

@yzguy yzguy commented Sep 27, 2017 •
edited
Loading

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

This is first ever OSS pull request to a real project, I figured it was a relatively small easy one.

This adds a new method to the DSL parsing to expose the languages specified in a cask, and a new section in the info output for available languages

-> brew cask info firefox
firefox: 55.0.3
https://www.mozilla.org/firefox/
Not installed
From: https://github.com/caskroom/homebrew-cask/blob/master/Casks/firefox.rb
==> Name
Mozilla Firefox
==> Languages
cs, de, en-GB, en, es-AR, es-CL, es-ES, fi, fr, gl, in, it, ja, nl, pl, pt, pt-BR, ru, uk, zh-TW, zh
==> Artifacts
Firefox.app (App)

The section does not show up if there are no languages specified in the cask

-> brew cask info 1password
1password: 6.8.2
https://1password.com/
Not installed
From: https://github.com/caskroom/homebrew-cask/blob/master/Casks/1password.rb
==> Name
1Password
==> Artifacts
1Password 6.app (App)

Fixes Homebrew/homebrew-cask#31128

I'm absolutely open to any help, suggestions, criticism.

reitermarkus
reitermarkus reviewed Sep 27, 2017
View reviewed changes
Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

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

Would be great if you could add a small test for this. Thanks!

@reitermarkus reitermarkus self-assigned this Sep 27, 2017
@yzguy
Copy link
Contributor Author

yzguy commented Sep 28, 2017 •
edited
Loading

Absolutely. If you don't mind, I'd like to bounce my thoughts off you before I tackle it. Testing takes a little for me to wrap my head around what is a good test.

By changing Library/Homebrew/cask/lib/hbc/cli/info.rb, we would want a test in test/cask/cli/info_spec.rb and by changing Library/Homebrew/cask/lib/hbc/dsl.rb, we would want to add a test in test/cask/dsl.rb.

For test/cask/cli/info_spec.rb, we want a test to check if that a cask specified multiple languages, it would print them, then another checking that nothing is printed if it did not specify any.

For test/cask/dsl.rb, we want a test under the describe "language stanza" do that checks if any language blocks are specified, we get a flat array of them. If none are specified, we get an empty array.

@reitermarkus
Copy link
Member

reitermarkus commented Sep 29, 2017

Yes, these sound exactly like the tests we'd need. 👍

@yzguy
add available languages to cask info command
9231190 
add language tests for dsl

add fixtures, tests for languages info output

add extra lines
@yzguy
Copy link
Contributor Author

yzguy commented Oct 1, 2017

Tests added, brew tests on my machine passed, TravisCI tests looked to have passed to. I think that should be everything :)

reitermarkus
reitermarkus requested changes Oct 2, 2017
View reviewed changes
Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

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

Some stylistic changes, other than that 👍 .

Library/Homebrew/test/cask/dsl_spec.rb Outdated
expect(cask.call.url.to_s).to eq("https://example.org/en-US.zip")
end

it "returns empty array if no languages specified" do
Copy link
Member

@reitermarkus reitermarkus Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

an empty array … are specified

Library/Homebrew/test/cask/cli/info_spec.rb Outdated
EOS
end

it 'should not print "Languages" section divider if the languages block has no output' do
Copy link
Member

@reitermarkus reitermarkus Oct 2, 2017

Choose a reason for hiding this comment

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

does not print

Library/Homebrew/test/cask/cli/info_spec.rb Outdated
EOS
end

it "should print languages if the Cask provided any" do
Copy link
Member

@reitermarkus reitermarkus Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

prints languages specified in the Cask

Library/Homebrew/test/support/fixtures/cask/Casks/with-conditional-languages.rb Outdated
@@ -0,0 +1,9 @@
cask 'with-conditional-languages' do
Copy link
Member

@reitermarkus reitermarkus Oct 2, 2017

Choose a reason for hiding this comment

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

without-languages

@reitermarkus reitermarkus merged commit ec0d8fa into Homebrew:master Oct 3, 2017
@reitermarkus
Copy link
Member

reitermarkus commented Oct 3, 2017

Great work, @yzguy!

@yzguy yzguy deleted the yzguy/cask_available_languages_to_info branch October 3, 2017 16:39
@yzguy
Copy link
Contributor Author

yzguy commented Oct 3, 2017

Thank you!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@reitermarkus reitermarkus reitermarkus approved these changes

Assignees

@reitermarkus reitermarkus

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yzguy @reitermarkus