Variable configure file #4

zonuexe · 2018-04-11T16:08:48Z

refs #2

zonuexe · 2018-04-11T16:18:44Z

Probably this change conflicts with #3.
This variable is not a customization variable, but it maybe works fine.

zonuexe · 2018-04-11T16:37:00Z

test.php

+// Local Variables:
+// phpstan-config-file: (root . "tests/phpstan.neon")
+// phpstan-level: 7
+// End:


This test is not automated, but we can check flycheck's behavior manually.

zonuexe · 2018-04-11T16:42:39Z

phpstan.el

-              "analyze" 
-              "--no-progress"
-              "--errorFormat=raw" 
+    :command ("php" (eval (phpstan-get-executable))


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.

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

Thanks. This problem was solved with the hack of phpstan-enabled-and-set-flycheck-variable. It is an evil side effect.

Fuco1 · 2018-04-12T08:41:30Z

phpstan.el

+
+;;;###autoload
+(progn
+  (defvar phpstan-config-file nil)


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.

Fuco1 · 2018-04-12T08:42:25Z

phpstan.el

+       #'(lambda (v) (or (null v)
+                         (integerp v)
+                         (and (stringp v)
+                              (string-match-p "\\`[1-9][0-9]*\\'" v))))))


This does not match "0", why?

Thanks. It fixed.

Fuco1 · 2018-04-12T08:42:55Z

phpstan.el

+
+;;;###autoload
+(progn
+  (defvar phpstan-executable nil)


Flycheck defines a variable for this automatically. I guess the question is the same as for the variable above.

Fuco1 · 2018-04-12T08:43:57Z

phpstan.el

+  "Return path to phpstan configure file or `NIL'."
+  (if phpstan-config-file
+      (if (and (consp phpstan-config-file)
+               (eq 'root (car phpstan-config-file)))


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.

Fuco1 · 2018-04-12T08:45:00Z

phpstan.el

-              "analyze" 
-              "--no-progress"
-              "--errorFormat=raw" 
+    :command ("php" (eval (phpstan-get-executable))


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

Fuco1 · 2018-04-12T08:45:40Z

phpstan.el

+    :command ("php" (eval (phpstan-get-executable))
+              "analyze" "--errorFormat=raw" "--no-progress" "--no-interaction"
+              "-c" (eval (phpstan-get-config-file))
+              "-l" (eval (phpstan-get-level))


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.

Fuco1 · 2018-04-12T08:46:19Z

phpstan.el

              source)
    :working-directory (lambda (_) (php-project-get-root-dir))
-    :enabled (lambda () (locate-dominating-file "phpstan.neon" default-directory))
+    :enabled (lambda () (phpstan-get-config-file))


I think this still doesn't work if I don't use any configuration file at all.

…ecutable

Fuco1 · 2018-04-12T15:50:16Z

phpstan.el

+
+;;;###autoload
+(progn
+  (defvar phpstan-config-file nil)


Flycheck also provides the flycheck-def-config-file-var macro for this

zonuexe · 2018-04-13T16:44:44Z

@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 of phpstan-config-file should 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.

zonuexe · 2018-04-15T00:26:01Z

This code needs to be refactored, but once it is merged into master it will be the first release.

Fuco1 · 2018-04-15T09:20:27Z

phpstan.el

-              source)
-    :working-directory (lambda (_) (php-project-get-root-dir))
-    :enabled (lambda () (locate-dominating-file "phpstan.neon" default-directory))
+    :command ("php" (eval (phpstan-get-command-args))


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?

@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.

Fuco1 · 2018-04-15T09:23:26Z

@zonuexe Thanks, I think your points are valid, if we want to support other mechanisms than flycheck then it makes sense to me.

zonuexe added 9 commits April 12, 2018 00:17

composer require --dev phpstan/phpstan:dev-master

7bf0f23

Add dependency php-mode in development

ba7dfc8

Add file local variables and getter functions

526f0b7

Consider the existence of "phpstan.neon.dist"

968b8fe

Refactor using cl-loop

c48251b

Modify flycheck checker

a5f1711

Rename flycheck checker

45ba779

Add test files

bc56031

Use config-file instead of configure-file

0825ac8

zonuexe requested a review from Fuco1 April 11, 2018 16:11

zonuexe commented Apr 11, 2018

View reviewed changes

zonuexe mentioned this pull request Apr 11, 2018

Add flycheck options #3

Closed

zonuexe assigned Fuco1 Apr 11, 2018

zonuexe added 2 commits April 12, 2018 10:14

Remove unused variable

6fa077b

Fix phpstan executable

6840f6c

Fuco1 reviewed Apr 12, 2018

View reviewed changes

zonuexe added 5 commits April 12, 2018 20:06

Fix pattern of level (accepts 1 digit number)

a198897

Add defgroup phpstan and custom variable phpstan-flycheck-auto-set-ex…

63fd9eb

…ecutable

Modify phpstan-executable specification

4555592

Add phpstan-get-config-file-and-set-flycheck-variable as side effect

d559ee1

Add phpstan-normalize-path and support Docker

05c9f76

Fuco1 reviewed Apr 12, 2018

View reviewed changes

zonuexe mentioned this pull request Apr 14, 2018

Feature request : integrate phpactor emacs-php/php-mode#451

Open

zonuexe added 5 commits April 15, 2018 09:20

Add 'docker keyword

70cb256

Normalize phpstan-config-file for Docker

23d1860

Add 'max keyword to phpstan-level

dffcad7

Add phpstan-working-dir for working directory of PHPStan

3748533

Rename root-directory local variable

4d53e60

zonuexe added 4 commits April 15, 2018 09:20

Search phpstan.neon from (phpstan-get-working-dir)

56355b0

Modify doc comment and README

21cf970

Modify phpstan-enabled-and-set-flycheck-variable

8d63798

phpstan-get-executable return always executable with arguments

0508372

zonuexe force-pushed the feature/variable-configure-file branch from bfaf09e to 0508372 Compare April 15, 2018 00:23

zonuexe merged commit 8a95d94 into master Apr 15, 2018

zonuexe deleted the feature/variable-configure-file branch April 15, 2018 00:26

Fuco1 reviewed Apr 15, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variable configure file #4

Variable configure file #4

zonuexe commented Apr 11, 2018

zonuexe commented Apr 11, 2018

zonuexe Apr 11, 2018

zonuexe Apr 11, 2018

Fuco1 Apr 12, 2018

zonuexe Apr 13, 2018

Fuco1 Apr 12, 2018

Fuco1 Apr 12, 2018

zonuexe Apr 13, 2018

Fuco1 Apr 12, 2018

Fuco1 Apr 12, 2018

Fuco1 Apr 12, 2018

Fuco1 Apr 12, 2018

Fuco1 Apr 12, 2018

Fuco1 Apr 12, 2018

zonuexe commented Apr 13, 2018

zonuexe commented Apr 15, 2018

Fuco1 Apr 15, 2018

zonuexe Apr 15, 2018

Fuco1 commented Apr 15, 2018

Variable configure file #4

Variable configure file #4

Conversation

zonuexe commented Apr 11, 2018

zonuexe commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonuexe commented Apr 13, 2018

zonuexe commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fuco1 commented Apr 15, 2018