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

caveats.rb: empty method on Linux #3315

Merged
merged 3 commits into from Oct 18, 2017

Conversation

Projects
None yet
3 participants
@maxim-belkin
Contributor

maxim-belkin commented Oct 13, 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?

plist_caveats does not apply to Linux so we have to override it to an empty method.

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 13, 2017

Contributor

Forgot to mention @sjackman to chime in/check

Contributor

maxim-belkin commented Oct 13, 2017

Forgot to mention @sjackman to chime in/check

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 13, 2017

Contributor

Thanks for this work, Maxim. A bit more work is needed. Move the existing plist_caveats method to a file extend/os/mac/caveats.rb. Create a file extend/os/caveats.rb with contents

require "extend/os/mac/caveats" if OS.mac?

In Library/Homebrew/caveats.rb add the method def plist_caveats; end To the end of this file add

require "extend/os/caveats"

No need for the file extend/os/linux/caveats.rb

Contributor

sjackman commented Oct 13, 2017

Thanks for this work, Maxim. A bit more work is needed. Move the existing plist_caveats method to a file extend/os/mac/caveats.rb. Create a file extend/os/caveats.rb with contents

require "extend/os/mac/caveats" if OS.mac?

In Library/Homebrew/caveats.rb add the method def plist_caveats; end To the end of this file add

require "extend/os/caveats"

No need for the file extend/os/linux/caveats.rb

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 13, 2017

Contributor

OK, will do

If only there was a variable to add a single line, instead of 5

require "extend/os/#{os}/caveats"
Contributor

maxim-belkin commented Oct 13, 2017

OK, will do

If only there was a variable to add a single line, instead of 5

require "extend/os/#{os}/caveats"
@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 13, 2017

Contributor

There's ENV["HOMEBREW_SYSTEM"] which has the value Macintosh.
I'd suggest creating a global variable in os.rb named OS::SYSTEM that takes the values mac, linux, and generic.

The file "extend/os/#{os}/caveats" may not exist, so it'd have to be something like…

require "extend/os/#{OS::SYSTEM}/system_config" if (HOMEBREW_REPOSITORY/"extend/os"/OS::SYSTEM/"system_config.rb").readable?

@MikeMcQuaid Your thought on the above, rather than the current…

if OS.mac?
  require "extend/os/mac/system_config"
elsif OS.linux?
  require "extend/os/linux/system_config"
end
Contributor

sjackman commented Oct 13, 2017

There's ENV["HOMEBREW_SYSTEM"] which has the value Macintosh.
I'd suggest creating a global variable in os.rb named OS::SYSTEM that takes the values mac, linux, and generic.

The file "extend/os/#{os}/caveats" may not exist, so it'd have to be something like…

require "extend/os/#{OS::SYSTEM}/system_config" if (HOMEBREW_REPOSITORY/"extend/os"/OS::SYSTEM/"system_config.rb").readable?

@MikeMcQuaid Your thought on the above, rather than the current…

if OS.mac?
  require "extend/os/mac/system_config"
elsif OS.linux?
  require "extend/os/linux/system_config"
end
@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 13, 2017

Contributor
require "extend/os/#{os}/caveats" if (HOMEBREW_REPOSITORY/"extend/os"/os/"caveats.rb").readable?

Is it possible / OK to "extend" the require method?

Contributor

maxim-belkin commented Oct 13, 2017

require "extend/os/#{os}/caveats" if (HOMEBREW_REPOSITORY/"extend/os"/os/"caveats.rb").readable?

Is it possible / OK to "extend" the require method?

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 13, 2017

Contributor

I wouldn't extend it, but you could define a new method require_if_available (or some name).

Contributor

sjackman commented Oct 13, 2017

I wouldn't extend it, but you could define a new method require_if_available (or some name).

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 16, 2017

Contributor

Made the changes without creating any new variable(s).

Contributor

maxim-belkin commented Oct 16, 2017

Made the changes without creating any new variable(s).

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 16, 2017

Contributor
Library/Homebrew/extend/os/mac/caveats.rb:45:1: C: Extra empty line detected at class body end.

https://travis-ci.org/Homebrew/brew/jobs/288642476#L860

Contributor

sjackman commented Oct 16, 2017

Library/Homebrew/extend/os/mac/caveats.rb:45:1: C: Extra empty line detected at class body end.

https://travis-ci.org/Homebrew/brew/jobs/288642476#L860

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 16, 2017

Contributor

Yes, I noticed that after I pushed it :)

Contributor

maxim-belkin commented Oct 16, 2017

Yes, I noticed that after I pushed it :)

@maxim-belkin

This comment has been minimized.

Show comment
Hide comment
@maxim-belkin

maxim-belkin Oct 16, 2017

Contributor

Fixed!

Contributor

maxim-belkin commented Oct 16, 2017

Fixed!

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 18, 2017

Member

Thanks again @maxim-belkin!

Member

MikeMcQuaid commented Oct 18, 2017

Thanks again @maxim-belkin!

@MikeMcQuaid MikeMcQuaid merged commit dc9ea9c into Homebrew:master Oct 18, 2017

3 checks passed

codecov/patch 100% of diff hit (target 67.43%)
Details
codecov/project Absolute coverage decreased by -0.23% but relative coverage increased by +32.56% compared to c3ff748
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxim-belkin maxim-belkin deleted the maxim-belkin:linux-caveats-plist branch Oct 19, 2017

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