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.

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

dev-cmd/bottle: fix positioning of bottle block in some cases #7875

Merged
merged 1 commit into from Jul 2, 2020

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Jul 1, 2020

  • 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 style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Fixes the bottle commit breaking brew style when the license line or livecheck block is included.

@Rylan12
Copy link
Member

Rylan12 commented Jul 1, 2020

Just curious, the components_order rubocop has a few additional components that precede the bottle block:

def audit_formula(_node, _class_node, _parent_class_node, body_node)
component_precedence_list = [
[{ name: :include, type: :method_call }],
[{ name: :desc, type: :method_call }],
[{ name: :homepage, type: :method_call }],
[{ name: :url, type: :method_call }],
[{ name: :mirror, type: :method_call }],
[{ name: :version, type: :method_call }],
[{ name: :sha256, type: :method_call }],
[{ name: :license, type: :method_call }],
[{ name: :revision, type: :method_call }],
[{ name: :version_scheme, type: :method_call }],
[{ name: :head, type: :method_call }],
[{ name: :stable, type: :block_call }],
[{ name: :livecheck, type: :block_call }],
[{ name: :bottle, type: :block_call }],

Namely: livecheck, stable, head, url and include.

Should these be added here as well?

Also, can sha1 be removed given that Homebrew has removed sha1 support as of ~4 years ago?

@Bo98
Copy link
Member Author

Bo98 commented Jul 1, 2020

url and head is already there.

I will test the others.

Also, can sha1 be removed given that Homebrew has removed sha1 support as of ~4 years ago?

Probably.

@Bo98 Bo98 force-pushed the license-bottle-block branch 2 times, most recently from 5eb3adc to d2146b8 Compare July 1, 2020 22:46
@Bo98
Copy link
Member Author

Bo98 commented Jul 1, 2020

stable do was also already supported but not very well (no blank line after the end), I've tweaked it now.

Regex on code is kinda horrible to do, but doing anything else is a bigger task than I expected to do here.

@Bo98 Bo98 changed the title dev-cmd/bottle: fix positioning of bottle block to be after license line dev-cmd/bottle: fix positioning of bottle block in some cases Jul 1, 2020
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good to me. This is probably something we should consider doing with brew style --fix instead. Pretty sure if you write the bottle block basically anywhere it'll put it in the right place.

@Bo98
Copy link
Member Author

Bo98 commented Jul 2, 2020

Yeah, that's an idea.

I'll merge as is to mitigate the impact it's currently having though.

@Bo98 Bo98 merged commit c1cbf1a into Homebrew:master Jul 2, 2020
@Bo98 Bo98 deleted the license-bottle-block branch July 2, 2020 17:11
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 26, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants