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.

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

Add language method to OS::Mac. #897

Merged
merged 1 commit into from
Sep 9, 2016
Merged

Add language method to OS::Mac. #897

merged 1 commit into from
Sep 9, 2016

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Sep 8, 2016 •
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 adds a language method to the MacOS module, to be used in casks, such as Homebrew/homebrew-cask#24343.

cc @Homebrew/cask

@reitermarkus reitermarkus added the in progress Maintainers are working on this label Sep 8, 2016
@reitermarkus reitermarkus changed the title Add language method to OS:Mac. Add language method to OS::Mac. Sep 8, 2016
DomT4
DomT4 reviewed Sep 9, 2016
View reviewed changes
Library/Homebrew/os/mac.rb
@@ -41,6 +41,10 @@ def cat
version.to_sym
end

def language
@language ||= `defaults read .GlobalPreferences AppleLanguages`.delete(" \n\"()").sub(%r{,.*}, '')
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Utils.popen_read instead here? Backticks are inconsistent with the rest of the file here... and kind of gross to look at 😄.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Also is it worth using gsub? I'd also favour // regexes unless they contain /.

Copy link
Member Author

@reitermarkus reitermarkus Sep 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

You mean gsub instead of .delete.sub or instead of only .sub?

Example output is:

(
  "de-AT",
  "en-GB",
  en
)

So delete gives you a nice CSV-like string: "de-AT,en-GB,en"

Copy link
Member

Choose a reason for hiding this comment

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

Either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving as is because I don't think having a complex regex here is worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Feels weird on hindsight to return a comma-separated string. .delete("()\"\n ").split(",") seems to do what you want and return an array.

@MikeMcQuaid
Copy link
Member

Feels a little weird for this to be on the MacOS class but I don't have strong feelings either way if there's no other logical place.

@reitermarkus
Copy link
Member Author

Feels a little weird for this to be on the MacOS class

Where else would you put the OS language, if not in the OS module?

@MikeMcQuaid
Copy link
Member

Where else would you put the OS language, if not in the OS module?

👍

DomT4
DomT4 reviewed Sep 9, 2016
View reviewed changes
Library/Homebrew/os/mac.rb
@@ -41,6 +41,10 @@ def cat
version.to_sym
end

def language
@language ||= Utils.popen_read('defaults', 'read', '.GlobalPreferences', 'AppleLanguages').delete(" \n\"()").sub(/,.*/, '')
Copy link
Member

Choose a reason for hiding this comment

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

Single quotes is probably a Rubocop violation given Homebrew's Rubocop config?

Copy link
Member Author

@reitermarkus reitermarkus Sep 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Speaking of RuboCop, when was the last time someone actually ran it? Seems there are hundreds of violations.

Edit: I ran it in Library/Homebrew instead of Library. It seems to be broken anyway because of the dependency on rubocop-cask.

Copy link
Member

Choose a reason for hiding this comment

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

We mass-fixed core code pre-separation, so a while ago. There is stuff Rubocop skips over automatically fixing though, as well as us having changed various Rubocop rules between then & now.

@reitermarkus reitermarkus force-pushed the os-language branch 2 times, most recently from e414e63 to 60a4060 Compare September 9, 2016 16:54
@reitermarkus
Copy link
Member Author

Ok, should be good to go now.

@reitermarkus
Copy link
Member Author

Merging now in order for Homebrew/homebrew-cask#24343 and Homebrew/homebrew-cask-versions#2558 to hopefully go green.

@reitermarkus reitermarkus merged commit 4fb691e into Homebrew:master Sep 9, 2016
@reitermarkus reitermarkus removed the in progress Maintainers are working on this label Sep 9, 2016
@MikeMcQuaid
Copy link
Member

@reitermarkus Did you miss this:

Feels weird on hindsight to return a comma-separated string. .delete("()\"\n ").split(",") seems to do what you want and return an array.

I'd have preferred if you disagreed that it was discussed rather than merged?

@jawshooah
Copy link
Contributor

It doesn't return a comma-separated string. The sub method call strips away the first comma and everything after it, leaving only the first value.

@MikeMcQuaid
Copy link
Member

Ok. Why does this only care about the first value? That feels relatively opinionated for something that's a OS wrapper around a method? Regardless: this is discussion we should really be having before this was merged.

@reitermarkus
Copy link
Member Author

reitermarkus commented Sep 9, 2016 •
edited
Loading

@reitermarkus Did you miss this:

Yes, sorry, I missed that comment.

For context, please take a look at https://github.com/caskroom/homebrew-cask/pull/24343/files#diff-d7a1b27ed779bc1601aadf2a1c3652f7R12 on how this is supposed to be used.

Alternatively, to make this less opinionated, we could make MacOS.languages return an array, and MacOS.language return languages.first.

@MikeMcQuaid
Copy link
Member

@reitermarkus No worries. 👍 to having languages return an array and language return languages.first 👍

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reitermarkus @MikeMcQuaid @jawshooah @DomT4