Make `MacOS.language` less opinionated and add `language` stanza. #906

reitermarkus · 2016-09-10T03:07:17Z

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 adds a languages method which returns an array, which in turn is called by language.

A second addition is a language stanza to the cask DSL, which will hopefully make it easier to create and maintain multilingual casks.

MikeMcQuaid · 2016-09-10T08:43:22Z

👍 thanks!

reitermarkus · 2016-09-15T01:01:14Z

Seems like the Mavericks test-bot is using zh-Hans for some reason. So the regex in the tests should also match http://www.unicode.org/iso15924/iso15924-codes.html.

reitermarkus · 2016-09-17T02:03:31Z

@Homebrew/cask, could someone look over this?

reitermarkus · 2016-09-17T02:06:52Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+      if args.include?(:default)
+        # :default has to be the last language block in order to assign return value of the selected `language` block to `@language`
+        @language = @language[:block].call
+      end


I'm sure there is a better place/way to do this. Could need some help here, @jawshooah and @mwean.

UniqMartin · 2016-09-17T15:28:11Z

Library/Homebrew/os/mac.rb

@@ -41,8 +41,12 @@ def cat
      version.to_sym
    end

+    def languages
+      @languages ||= Utils.popen_read("defaults", "read", ".GlobalPreferences", "AppleLanguages").delete(" \n\"()").split(",")


How about the following?

Utils.popen_read("defaults", "read", ".GlobalPreferences", "AppleLanguages").scan(/[^ \n"(),]+/)

MikeMcQuaid · 2016-09-17T19:43:49Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+          next unless l.match(string_or_regex)
+          next unless @language[:level].nil? || @language[:level] > index
+          @language = {
+                        block: block,


Up to you but I'd pull this back to being two-chars in from @language.

This is the style according to the current RuboCop config for Cask, so I'm leaving this as-is for now as it is used throughout cask/*.

🆒. Would be good after #989 to try and get a single style and rubocop/brew style invocation working for both so we can add brew style to CI.

MikeMcQuaid · 2016-09-17T19:44:18Z

Library/Homebrew/os/mac.rb

@@ -41,8 +41,12 @@ def cat
      version.to_sym
    end

+    def languages
+      @languages ||= Utils.popen_read("defaults", "read", ".GlobalPreferences", "AppleLanguages").scan(/[^ \n"(),]+/)


mwean · 2016-09-17T20:24:15Z

I'm trying to work on this, but I'm having a ton of trouble getting things working. The develop_brew_cask command isn't working, and I can't run the tests because of missing environment variables. I'm on Gitter if anyone can help me.

reitermarkus · 2016-09-17T21:34:48Z

Never used the develop_brew_cask command, but this will probably need to be rewritten or be removed, as developing only Cask will not work as before anyways.

UniqMartin · 2016-09-18T20:15:45Z

Library/Homebrew/os/mac.rb

    def language
-      @language ||= Utils.popen_read("defaults", "read", ".GlobalPreferences", "AppleLanguages").delete(" \n\"()").sub(/,.*/, "")
+      languages.first


This will return nil if there's no default language; previously it returned an empty string. I guess it doesn't matter in practice as there's a slim (or even zero?) chance of this macOS setting to be set to an empty array. Still wanted to point this out.

there's a slim (or even zero?) chance of this macOS setting to be set to an empty array

There is only one way I can think of this could be the case: Manually changing/deleting .GlobalPreferences, which is highly unlikely, so I think we're good here.

MikeMcQuaid · 2016-09-20T07:25:53Z

@reitermarkus 🚢?

reitermarkus · 2016-09-20T07:28:35Z

Would still like to get some feedback from @Homebrew/cask.

mwean · 2016-09-20T17:21:35Z

I'd love to help, but I can't get things running locally. I'd also love to have some tests around this. I also don't like that we have to make :default the last declaration.

reitermarkus · 2016-09-20T17:39:36Z

I'd also love to have some tests around this.

Good point, this still needs tests.

I also don't like that we have to make :default the last declaration.

This is not the case anymore. This is done using the language_eval method now.

mwean · 2016-09-26T00:52:40Z

@reitermarkus I made a slightly different DSL: reitermarkus/brew@os-language...mwean:mw/language-dsl I wanted to avoid having a separate default block, and I wanted to make it easy to make the language name different from the pattern.

reitermarkus · 2016-09-27T20:27:31Z

@mwean, I like the idea of only allowing strings in the DSL, so I have added a Locale class to enable matching locales by string only instead of regexes and strings. Still needs to be integrated into the language DSL.

reitermarkus · 2016-10-01T01:31:32Z

I think this is pretty much ready. @mwean, I changed it mostly to your DSL, except for the name: parameter, as I think simply using the return value of the block is more flexible, and more in line with url do. @jawshooah, please also have a final look.

jawshooah

Just a couple of style nits, but all in all this LGTM 👍

jawshooah · 2016-10-02T21:33:37Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+
+        @language_blocks.default = block
+      else
+        language_eval


You could clean this up a bit by making language_eval return a value instead of having it set @language. So here you'd have @language ||= language_eval...

I'll leave this as is because of https://github.com/Homebrew/brew/pull/906/files/3def3b805b139960f11af68e07e3295c6dbe92cb#diff-f85d4948a976b9727959680dbfe1118aR16.

Otherwise I'd have to call @dsl.language to actually “evaluate” the languages, which kind of defeats the purpose of having a language_eval method.

jawshooah · 2016-10-02T21:33:58Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+    end
+
+    def language_eval
+      return if instance_variable_defined?(:@language)


...and you could get rid of this early return...

jawshooah · 2016-10-02T21:34:15Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+          strings.any? { |string| locale.include?(string) }
+        }
+
+        return @language = @language_blocks[key].call unless key.nil?


...and you could remove the @language = here...

jawshooah · 2016-10-02T21:34:22Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+        return @language = @language_blocks[key].call unless key.nil?
+      }
+
+      @language = @language_blocks.default.call


...and here.

jawshooah · 2016-10-02T22:27:33Z

Library/Homebrew/cask/lib/hbc/dsl.rb

+
+      return unless instance_variable_defined?(:@language_blocks)
+
+      MacOS.languages.map(&Locale.method(:parse)).any? { |locale|


Seems like each is a better fit here than any? since you're not using the return value, as a boolean or otherwise.

reitermarkus added the in progress Maintainers are working on this label Sep 10, 2016

reitermarkus force-pushed the os-language branch from ae20770 to 6bf7b18 Compare September 12, 2016 04:29

reitermarkus mentioned this pull request Sep 14, 2016

Convert casks to be multilingual. Homebrew/homebrew-cask#24343

Merged

reitermarkus force-pushed the os-language branch 2 times, most recently from b21d419 to 1c748c6 Compare September 15, 2016 00:38

reitermarkus force-pushed the os-language branch from ef08490 to fda249e Compare September 17, 2016 02:05

reitermarkus commented Sep 17, 2016

View reviewed changes

UniqMartin reviewed Sep 17, 2016

View reviewed changes

reitermarkus force-pushed the os-language branch 2 times, most recently from beb8b3b to 4fb377b Compare September 17, 2016 17:51

MikeMcQuaid approved these changes Sep 17, 2016

View reviewed changes

reitermarkus force-pushed the os-language branch 3 times, most recently from 5c9d8bc to 1cd5384 Compare September 18, 2016 02:29

adidalal mentioned this pull request Sep 18, 2016

Add Adobe Premiere Pro Homebrew/homebrew-cask#24561

Merged

8 tasks

UniqMartin approved these changes Sep 18, 2016

View reviewed changes

reitermarkus changed the title ~~Make MacOS.language less opinionated.~~ Make MacOS.language less opinionated and add language stanza. Sep 20, 2016

reitermarkus mentioned this pull request Sep 21, 2016

Cleanup localized casks. Homebrew/homebrew-cask-versions#2558

Merged

reitermarkus force-pushed the os-language branch 3 times, most recently from e8daf87 to deead34 Compare September 21, 2016 20:39

reitermarkus force-pushed the os-language branch 4 times, most recently from 8a954fd to e745adf Compare September 25, 2016 22:07

reitermarkus force-pushed the os-language branch 2 times, most recently from c9b1e30 to 077e8fc Compare September 27, 2016 21:19

jawshooah approved these changes Oct 2, 2016

View reviewed changes

reitermarkus added 15 commits October 3, 2016 02:34

Make MacOS.language less opinionated.

1a25f1e

Add test for MacOS.languages.

929c594

Add language stanza to cask DSL.

f52116c

Add language_eval method.

65fdfef

Add test for language stanza.

87299af

Always fall back to language :default.

1e86c7d

Stub MacOS.languages instead of setting HOMEBREW_LANGUAGES.

b703c81

Add —language option.

546a91f

Refactor logic to always choose first matched language.

bc2d676

Support audit for multilingual casks.

bc143bb

Add Locale class to enable easier matching of locales.

b5531e8

Change language DSL to only allow strings.

013f33b

Raise error if more than one default language if specified.

b104e6f

Refactor audit for changed DSL.

ef26bf1

Style changes in language_eval.

e2b3753

reitermarkus force-pushed the os-language branch from 3def3b8 to e2b3753 Compare October 3, 2016 00:58

reitermarkus merged commit 35ee283 into Homebrew:master Oct 3, 2016

reitermarkus removed the in progress Maintainers are working on this label Oct 3, 2016

reitermarkus deleted the os-language branch October 3, 2016 02:03

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `MacOS.language` less opinionated and add `language` stanza. #906

Make `MacOS.language` less opinionated and add `language` stanza. #906

reitermarkus commented Sep 10, 2016 •

edited

Loading

MikeMcQuaid commented Sep 10, 2016

reitermarkus commented Sep 15, 2016 •

edited

Loading

reitermarkus commented Sep 17, 2016

reitermarkus Sep 17, 2016 •

edited

Loading

UniqMartin Sep 17, 2016

MikeMcQuaid Sep 17, 2016

reitermarkus Sep 17, 2016

MikeMcQuaid Sep 17, 2016

MikeMcQuaid Sep 17, 2016

mwean commented Sep 17, 2016 •

edited

Loading

reitermarkus commented Sep 17, 2016

UniqMartin Sep 18, 2016 •

edited

Loading

reitermarkus Sep 19, 2016 •

edited

Loading

MikeMcQuaid commented Sep 20, 2016

reitermarkus commented Sep 20, 2016

mwean commented Sep 20, 2016

reitermarkus commented Sep 20, 2016 •

edited

Loading

mwean commented Sep 26, 2016

reitermarkus commented Sep 27, 2016

reitermarkus commented Oct 1, 2016

jawshooah left a comment

jawshooah Oct 2, 2016

reitermarkus Oct 3, 2016

jawshooah Oct 2, 2016

jawshooah Oct 2, 2016

jawshooah Oct 2, 2016

jawshooah Oct 2, 2016


		return unless instance_variable_defined?(:@language_blocks)

		MacOS.languages.map(&Locale.method(:parse)).any? { \|locale\|

Make MacOS.language less opinionated and add language stanza. #906

Make MacOS.language less opinionated and add language stanza. #906

Conversation

reitermarkus commented Sep 10, 2016 • edited Loading

MikeMcQuaid commented Sep 10, 2016

reitermarkus commented Sep 15, 2016 • edited Loading

reitermarkus commented Sep 17, 2016

reitermarkus Sep 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwean commented Sep 17, 2016 • edited Loading

reitermarkus commented Sep 17, 2016

UniqMartin Sep 18, 2016 • edited Loading

Choose a reason for hiding this comment

reitermarkus Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 20, 2016

reitermarkus commented Sep 20, 2016

mwean commented Sep 20, 2016

reitermarkus commented Sep 20, 2016 • edited Loading

mwean commented Sep 26, 2016

reitermarkus commented Sep 27, 2016

reitermarkus commented Oct 1, 2016

jawshooah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `MacOS.language` less opinionated and add `language` stanza. #906

Make `MacOS.language` less opinionated and add `language` stanza. #906

reitermarkus commented Sep 10, 2016 •

edited

Loading

reitermarkus commented Sep 15, 2016 •

edited

Loading

reitermarkus Sep 17, 2016 •

edited

Loading

mwean commented Sep 17, 2016 •

edited

Loading

UniqMartin Sep 18, 2016 •

edited

Loading

reitermarkus Sep 19, 2016 •

edited

Loading

reitermarkus commented Sep 20, 2016 •

edited

Loading