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

add available languages to cask info command #3220

Merged
merged 2 commits into from Oct 3, 2017

Conversation

Projects
None yet
2 participants
@yzguy
Contributor

yzguy commented Sep 27, 2017

  • 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.

@MikeMcQuaid MikeMcQuaid requested a review from reitermarkus Sep 27, 2017

@reitermarkus

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

@reitermarkus reitermarkus self-assigned this Sep 27, 2017

@yzguy

This comment has been minimized.

Show comment
Hide comment
@yzguy

yzguy Sep 28, 2017

Contributor

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.

Contributor

yzguy commented Sep 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Sep 29, 2017

Member

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

Member

reitermarkus commented Sep 29, 2017

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

add available languages to cask info command
add language tests for dsl

add fixtures, tests for languages info output

add extra lines
@yzguy

This comment has been minimized.

Show comment
Hide comment
@yzguy

yzguy Oct 1, 2017

Contributor

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

Contributor

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

Some stylistic changes, other than that 👍 .

Show outdated Hide outdated Library/Homebrew/test/cask/dsl_spec.rb Outdated
Show outdated Hide outdated Library/Homebrew/test/cask/cli/info_spec.rb Outdated
Show outdated Hide outdated Library/Homebrew/test/cask/cli/info_spec.rb Outdated
Show outdated Hide outdated .../Homebrew/test/support/fixtures/cask/Casks/with-conditional-languages.rb Outdated

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

3 checks passed

codecov/patch 100% of diff hit (target 67.13%)
Details
codecov/project 67.15% (+0.02%) compared to 700300a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 3, 2017

Member

Great work, @yzguy!

Member

reitermarkus commented Oct 3, 2017

Great work, @yzguy!

@yzguy yzguy deleted the yzguy:yzguy/cask_available_languages_to_info branch Oct 3, 2017

@yzguy

This comment has been minimized.

Show comment
Hide comment
@yzguy

yzguy Oct 3, 2017

Contributor

Thank you!

Contributor

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.