Skip to content

dev-cmd/create: use std_configure_args#10801

Merged
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
lexicol:patch-1
Mar 9, 2021
Merged

dev-cmd/create: use std_configure_args#10801
MikeMcQuaid merged 2 commits intoHomebrew:masterfrom
lexicol:patch-1

Conversation

@lexicol
Copy link
Copy Markdown
Contributor

@lexicol lexicol commented Mar 6, 2021

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

@@ -156,10 +156,7 @@ def install
system "cmake", ".", *std_cmake_args
<% elsif mode == :autotools %>
# Remove unrecognized options if warned by configure
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we link to https://rubydoc.brew.sh/Formula.html#std_configure_args-instance_method in this comment, so that users know what exactly is going into std_configure_args? Especially given that the comment might be asking them to remove options they don't even know are there...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should links be added for the other std_*_args also? Why add this link when there is already the link to https://rubydoc.brew.sh/Formula above?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should links be added for the other std_*_args also?

Perhaps, but that's less important than for std_configure_args. There's very little need to know what the other std_*_args say: you can just include them and they'll just work. Not sure we can say the same for std_configure_args.

Why add this link when there is already the link to https://rubydoc.brew.sh/Formula above?

Same reasoning as above.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed with @carlocab

@MikeMcQuaid MikeMcQuaid merged commit 7b5f036 into Homebrew:master Mar 9, 2021
@MikeMcQuaid
Copy link
Copy Markdown
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @lexicol!

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2021
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.

3 participants