Skip to content

Add bazel-build-bazel-command - #16

Open
jlisee wants to merge 1 commit into
bazel-contrib:masterfrom
jlisee:custom-bazel-command
Open

Add bazel-build-bazel-command#16
jlisee wants to merge 1 commit into
bazel-contrib:masterfrom
jlisee:custom-bazel-command

Conversation

@jlisee

@jlisee jlisee commented Aug 14, 2019

Copy link
Copy Markdown

Sometimes users have custom invocation scripts that eliminate the need
for a user to install bazel to build their project. This allow
bazel-build to use that invocation script directly.

@jlisee
jlisee requested a review from laurentlb as a code owner August 14, 2019 20:40
@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Comment thread bazel-build.el Outdated
Comment thread bazel-build.el Outdated
Comment thread bazel-build.el Outdated
Sometimes users have custom invocation scripts that eliminate the need
for a user to install bazel to build their project. This allow
`bazel-build` to use that invocation script directly.
@jlisee
jlisee force-pushed the custom-bazel-command branch from d837392 to fbae714 Compare August 15, 2019 19:46
@jlisee

jlisee commented Aug 24, 2019

Copy link
Copy Markdown
Author

@googlebot I signed it!

@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Comment thread bazel-build.el
:group 'languages)

(defcustom bazel-build-bazel-program "bazel"
"Name of the 'bazel' program for 'bazel-build' and 'bazel-test'."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: Quote Lisp symbols in docstrings `like-this' or ‘like-this’. See https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks, new to this part of elisp will fix this in a bit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

SG, thanks, please ping this thread once you're ready.

@tautomaton

Copy link
Copy Markdown
Contributor

Laurent, is this ready to merge?

@laurentlb

Copy link
Copy Markdown
Collaborator

The last reviewer comment was:

SG, thanks, please ping this thread once you're ready.

ping @jlisee

@jin

jin commented Dec 30, 2019

Copy link
Copy Markdown

I have a patch that does essentially the same thing. Can we merge this now, and fix the nit later?

@laurentlb

Copy link
Copy Markdown
Collaborator

@phst Shall we merge this?

@phst

phst commented Feb 16, 2020

Copy link
Copy Markdown
Collaborator

@phst Shall we merge this?

Yes, that's fine with me. We can fix the nits afterwards.

@laurentlb

Copy link
Copy Markdown
Collaborator

I can't merge now, there are conflicts.

@jlisee, could you update your branch?

Comment thread bazel-build.el

(defun bazel-build (target)
"Build a Bazel TARGET."
(interactive (list (bazel-build--read-target "bazel build ")))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be good to also have the prompts here use the the variable instead of hard-coding "bazel".

Comment thread bazel-build.el
"Name of the 'bazel' program for 'bazel-build' and 'bazel-test'."
:type 'string
:group 'bazel-build
:link '(url-link "https://bazel.build/"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this is the kind of thing that is often project local (i.e., in .dir-locals.el), it'd be useful to add a :safe field.

From the Emacs info page:

   When defining a user option using ‘defcustom’, you can set its
‘safe-local-variable’ property by adding the arguments ‘:safe FUNCTION’
to ‘defcustom’ (*note Variable Definitions::).  However, a safety
predicate defined using ‘:safe’ will only be known once the package that
contains the ‘defcustom’ is loaded, which is often too late.  As an
alternative, you can use the autoload cookie (*note Autoload::) to
assign the option its safety predicate, like this:

     ;;;###autoload (put 'VAR 'safe-local-variable 'PRED)

The safe value definitions specified with ‘autoload’ are copied into the
package’s autoloads file (‘loaddefs.el’ for most packages bundled with
Emacs), and are known to Emacs since the beginning of a session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants