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

Allow checker executables to be set to a relative path #1485

Merged
merged 1 commit into from Sep 7, 2018

Conversation

CyberShadow
Copy link
Contributor

Hi,

Essentially, my goal is to put (flycheck-CHECKER-executable . "relative/path/to/checker-script") in .dir-locals.el, include said checker-script with the project, and have things "just work".

Below is one way to address this, but I'm open to suggestions.

Previously, flycheck-CHECKER-executable could not be set to a relative
path. This is useful when a checker (compatible substitute or wrapper)
is included with the project.

The setting didn't work because the relative path was passed as-is to
executable-find, which returns nil for inputs containing a directory
component.

Address this by introducing flycheck--find-executable, a new function
which checks whether its input has a directory component before
forwarding to executable-find, and update flycheck-executable-find's
default value to point to flycheck--find-executable instead of
executable-find.

@fmdkdd
Copy link
Member

fmdkdd commented Jul 31, 2018

Thanks! A couple of questions:

  1. How did you solve the problem prior to this patch? By customizing flycheck-executable-find in .dir-locals.el to return a hardcoded path? By extending your PATH environment per project? Can you show code that did that previously?

  2. The patch allows for any relative path, but the documentation only talks about absolute paths. Which is it?

I think I like that it provides a simpler way to specify a local checker executable, but I'm not fond of the overloading of the checker command. It seems harmless in practice though.

cc @cpitclaudel

@CyberShadow
Copy link
Contributor Author

How did you solve the problem prior to this patch?

By placing a wrapper in my PATH which checks PWD and changes behavior depending on the detected current project. This was a solution only I benefited from, and not easily extendable to other project contributors.

The patch allows for any relative path, but the documentation only talks about absolute paths. Which is it?

Sorry, how did you come to the conclusion that the documentation only talks about absolute paths?

but I'm not fond of the overloading of the checker command

Sorry, what do you mean by this? Do you mean that the user needs to choose between using the provided checker wrapper (by accepting the dir-locals setting), or using the system-wide one (by ignoring said setting), but not both?

@fmdkdd
Copy link
Member

fmdkdd commented Jul 31, 2018

This was a solution only I benefited from, and not easily extendable to other project contributors.

Sure, I understand the convenience of the PR. I just wanted a concrete look at how much effort you would save.

Sorry, how did you come to the conclusion that the documentation only talks about absolute paths?

You're right, I misread. The docstrings are clear on that point.

Sorry, what do you mean by this?

I meant using the checker command property in two ways. Previously it had only one meaning: the name of the checker executable, and we relied on a properly set PATH. With this patch, the command can also be the path to the checker executable, and in that case we override the PATH mechanism.

It's a minor point, but assigning multiple meanings to definitions can create confusion down the road. Although, in that case you could argue that the way Python checkers now have python as command creates a precedent. But again, minor point ;)

@CyberShadow
Copy link
Contributor Author

CyberShadow commented Jul 31, 2018

With this patch, the command can also be the path to the checker executable, and in that case we override the PATH mechanism.

Ah, I see what you mean. I don't see the distinction myself, as the behavior is consistent with what you get when you type a command at a shell prompt, or when you execute a command is a shell script, or when you call the C execvp function: if the path contains a directory separator, it is used relative to the current directory, otherwise it is looked up in PATH. From this point, it seems that the previous behavior is unexpected - generally, places which allow specifying a command that is then searched in PATH also allow specifying a relative or absolute path to an executable.

@cpitclaudel
Copy link
Member

I meant using the checker command property in two ways. Previously it had only one meaning: the name of the checker executable, and we relied on a properly set PATH. With this patch, the command can also be the path to the checker executable, and in that case we override the PATH mechanism.

🤔 don't we already allow absolute paths there?

@CyberShadow
Copy link
Contributor Author

Ah, OK, so executable-find works with absolute paths but not relative paths. That's curious. Its documentation doesn't mention it, and I didn't think to check.

@fmdkdd
Copy link
Member

fmdkdd commented Jul 31, 2018

I don't see the distinction myself, as the behavior is consistent with what you get when you type a command at a shell prompt

That's a good point.

Ah, OK, so executable-find works with absolute paths but not relative paths.

In any case, you also want the relative paths to work, right?

@CyberShadow
Copy link
Contributor Author

In any case, you also want the relative paths to work, right?

Yes, that's my goal.

@fmdkdd
Copy link
Member

fmdkdd commented Jul 31, 2018

don't we already allow absolute paths there?

It works, but it's not documented as far as I can tell.

flycheck.el Outdated
(function :tag "Search executables with a custom function"))
:package-version '(flycheck . "0.25")
:risky t)

(defun flycheck--find-executable (executable)
Copy link
Member

Choose a reason for hiding this comment

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

Since we are exposing this function in a configuration variable, it should be public (no double hyphens --).

flycheck.el Outdated
The default is the standard `executable-find' function which
searches `exec-path'. You can customize this option to search
for checkers in other environments such as bundle or NixOS
The default is `flycheck--find-executable', which searches
Copy link
Member

Choose a reason for hiding this comment

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

You haven't changed the default value though, only one of the options for customize. I'm fine with changing the default since it should be backwards compatible.

If we change the default, we should bump the package-version to 32.

@CyberShadow
Copy link
Contributor Author

CyberShadow commented Jul 31, 2018

Fixed and fixed :)

Should I do the package-version bump, or should I leave that to a maintainer?

@cpitclaudel
Copy link
Member

cpitclaudel commented Jul 31, 2018

It works, but it's not documented as far as I can tell.

It seems to be fairly common: https://github.com/search?q=flycheck+executable&type=Code , so it'll be good to document it :)

Edit: the docstring already said this:

The value of this option is a function which is given the name or
path of an executable and shall return the full path to the
executable

flycheck.el Outdated
"-fno-caret-diagnostics" ; Do not visually indicate the source
; location
"-fno-diagnostics-show-option" ; Do not show the corresponding
; warning group
Copy link
Member

Choose a reason for hiding this comment

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

Why did these move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Those are a local workaround that snuck into the amended commit. Removed, sorry.

@fmdkdd
Copy link
Member

fmdkdd commented Aug 1, 2018

Should I do the package-version bump, or should I leave that to a maintainer?

Go ahead and bump it.

Please also add a line to CHANGES.rst to advertize this change.

flycheck.el Outdated
@@ -453,24 +453,39 @@ commands through `bundle exec', `nix-shell' or similar wrappers."
:package-version '(flycheck . "0.25")
:risky t)

(defcustom flycheck-executable-find #'executable-find
(defcustom flycheck-executable-find #'flycheck-find-executable
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super happy with the names (we have flycheck-executable-find, flycheck-find-executable, and executable-find). What do you think of flycheck-default-executable-find?

@cpitclaudel
Copy link
Member

This PR looks good to me, thanks @CyberShadow! I've made one suggestion regarding naming.

@CyberShadow
Copy link
Contributor Author

Sorry for the delayed response! I've renamed the function and bumped flycheck-executable-find's package-version as suggested.

@fmdkdd
Copy link
Member

fmdkdd commented Sep 4, 2018

Looks good. No idea what's going on with the cla-assistant though. Please squash your commits; maybe it will wake up the CLA.

Previously, flycheck-CHECKER-executable could not be set to a relative
path. This is useful when a checker (compatible substitute or wrapper)
is included with the project.

The setting didn't work because the relative path was passed as-is to
executable-find, which returns nil for inputs containing a directory
component.

Address this by introducing flycheck-default-executable-find, a new
function which checks whether its input has a directory component
before forwarding to executable-find, and update
flycheck-executable-find's default value to point to
flycheck-default-executable-find instead of executable-find.

As its default is being changed, also bump flycheck-executable-find's
package-version to 32
@fmdkdd fmdkdd merged commit 2dc6f45 into flycheck:master Sep 7, 2018
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.

None yet

3 participants