-
Notifications
You must be signed in to change notification settings - Fork 14
Variable configure file #4
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
Conversation
Probably this change conflicts with #3.
This variable is not a customization variable, but it maybe works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is not automated, but we can check flycheck's behavior manually.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since phpstan is generally a PHP script (or Phar archive), this command works fine. Therefore, it is not possible to specify shell scripts that are not PHP or "real" executables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a feature though? I run phpstan through docker so this would not work for me for example. And there are many people who do so as well: https://github.com/phpstan/docker-image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This problem was solved with the hack of phpstan-enabled-and-set-flycheck-variable. It is an evil side effect.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason of having these as variables instead of flycheck options? The flycheck macro expands to a defcustom so we can still change it manually just as a defvar but it probides good documentation and listing of options for users.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not match "0", why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. It fixed.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flycheck defines a variable for this automatically. I guess the question is the same as for the variable above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear what the structure of phpstan-config-file should be. I think user options should at least be defined as defcustoms with a type annotation.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a feature though? I run phpstan through docker so this would not work for me for example. And there are many people who do so as well: https://github.com/phpstan/docker-image
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if these options eval to nil, then the flag has no value and it errors out. The (option ..) expansion flycheck uses takes care of that.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this still doesn't work if I don't use any configuration file at all.
phpstan.el
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flycheck also provides the flycheck-def-config-file-var macro for this
@Fuco1
I'm sorry I almost passed through your reviews. But for your pointing out, I was able to take care of Docker and make it work.
First of all, since this package is phpstan.el instead of flycheck-phpstan.el, I did not want to depends on only Flycheck. Since Flymake has been improved in Emacs 26, I'd like to avoid dependency on Flycheck for a minimum Emacs environment.
#4 (comment)
What's the reason of having these as variables instead of flycheck options? The flycheck macro expands to a defcustom so we can still change it manually just as a defvar but it probides good documentation and listing of options for users.
#4 (comment)
It is not clear what the structure ofphpstan-config-fileshould be. I think user options should at least be defined as defcustoms with a type annotation.
Your point is rational. However, I do not think that it is appropriate that project-specific settings depend on customization mechanisms. That is one reason why I did not depend on flycheck-def-config-file-var.
(root . "path/to/dir/phpstan-docker.neon")
This notation is not publicly specified, but I do not think that it is a remarkably esoteric notation by Emacs users. ......Uh, I know that my English sentences are not good.
bfaf09e to
0508372
Compare
This code needs to be refactored, but once it is merged into master it will be the first release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work when this starts "php" but I set 'docker as executable? Then the arguments are going to be run --rm -v..., that won't work with php would it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fuco1 This is an insane hack.
phpstan-enabled-and-set-flycheck-variable invoked as :enabled function sets flycheck-phpstan-executable as its side effect when flycheck-phpstan-executable is not set. Since it uses the fact that :enabled is called beforehand, there is a possibility that it will be destroyed in the future with low probability.
It was possible to explicitly write flycheck-phpstan-executable in the user setting, but it was handled within this function as it is somewhat complicated. I am looking for ways to solve this problem besides this hack.
@zonuexe Thanks, I think your points are valid, if we want to support other mechanisms than flycheck then it makes sense to me.
refs #2