Add current working directory [:default-directory] property #973
Conversation
|
@sergv Wow, thanks for taking up 👍 Really awesome work; at a cursory look I really like how you did things here. Also, many many thinks for fixing all those stylistic issues I left behind I'll review and test this PR soon, but please be patient. I'm a little short of time this week, and likely won't get to review this PR until next week. |
|
I feel uneasy for hijacking this pull request with fix for #978, but the fix is slightly dependent on changes that introduced |
|
@sergv Please remove that change from this PR. It's unrelated, and while I'm all in favour of a Please let's discuss this separately, even if this means that the fix for #978 will be delayed until after this PR is merged. |
|
Okay, I removed the fix for #978 from here, it can wait until this PR is merged. |
| (list (flycheck-ert-resource-filename "foo") | ||
| absolute-fn | ||
| absolute-fn |
There was a problem hiding this comment.
This looks like a spurious whitespace change?
There was a problem hiding this comment.
Good catch but actually it isn't a whitespace change - the inline list gets compared against errors list, which has 4 entries now.
There was a problem hiding this comment.
Oh dear, I should have taken a better look. Sorry for the noise 😊
|
@sergv Can you please also remove b281f2c from this PR? While it's a good intention it clutters the diff and makes it harder to review the important parts of this PR. I've taken some time to think about the general structure of this PR, and I think that it makes the default directory too, well, "global". I don't think that it should be a property of generic syntax checkers—it only makes sense for command syntax checkers in my opinion. Hence I don't think it should be part of the syntax check structure as well. Instead, I'd prefer if Could you try to refactor the PR accordingly? |
|
@lunaryorn I was under the impression that unresolved stylistic problems of #813 were, at least in part, blocking the review of the PR. I can remove the fixes and move them to another PR or I can move the commit to be the last commit, so you'll be able to review meaningful part at the beginning without clutter. Which way would be the best? I'll try refactoring PR and move default-directory into flycheck-define-command-checker soon. |
|
@sergv Sure, but there are many stylistics changes in this PR that are not related to the actual diff. For instance, you added periods to a couple of comments that weren't touched by this PR in any other way. That's not a bad thing per se, but it's cluttering the PR a bit. It'd be great of you could limit the diff to those parts that are actually changed as part of implementing the default-directory property. If it's too hard though leave the stylistic fixes in; it's ok. Thanks for your great work on this PR! |
|
@lunaryorn I rebased all the changes on latest master. I also reordered commits so that stylistic fixes are at the top and can be easily ignored via |
| @@ -4257,6 +4263,27 @@ universal prefix arg, and only the id with normal prefix arg." | |||
| (`(eval ,_) t) | |||
| (_ nil))) | |||
|
|
|||
| (defun flycheck-default-directory-wrapper (checker) | |||
There was a problem hiding this comment.
I don't like the name of this function :) It's too generic.
Can we come up with a better one? E.g. flycheck-compute-default-directory, or so?
|
@lunaryorn now the button is green, thanks! I'll rebase and merge. |
This is a first draft and has no input checking. Is enough for static
use case.
:cwd will change the default-directory in execution code environment.
ie.: :cwd ("~/temp")
:environment will temporarily add environment variables and needs to
a list of key value pairs like so:
:environment ("var1=value1" "var2=value2")
…absolut paath from which the checker command will be executed.
It will live on separate branch cwd-env.
adding code so that flycheck-fix-error-filename can use relative file names, which are relative to cwd.
:cwd property has been renamed to :default-directory. Add doc strings and a check if defined :default-directory exists.
remove the sharp quote in flycheck-define-checker macro for passing on the default-directory property to flycheck-define-command checker. This avoids a problem encountered with lambda functions.
|
@lunaryorn However, it seems new approval is needed after rebase. Can you please approve again? |
|
@lunaryorn For some reason, github still created a merge commit. Not sure what to do about it. |
|
Now that we have this merged, we can make |
|
@sergv Merge commits are fine for larger branches such as this 😊 @mrkkrp This branch adds a new default-directory property to syntax checker definitions. Refer to the docstrings for details. I don't plan to update any syntax checkers following this change but if you think it's reasonable to change some syntax checkers please go ahead an open a pull request. |
Pass it the syntax checker being used to allow dispatching on the checker and permit it to return nil to fall back to the normal default directory. See GH-973
|
@sergv I've changed the interface of the Meanwhile, would you please take a look at #1005? This change seems to have broken generic syntax checkers. |
|
@lunaryorn The change to @lunaryorn @mrkkrp Regarding default-directory property for |
|
@sergv Yes the macro should support all properties of flycheck-define-generic-checker. It'd be great if you could make this change as well. |
Hi, this is a followup to #813. I cannot add new commits to that pull request so I'm creating a new one.
Since original pull request was stalled for quite some time with several unresolved review comments I decided to jump in and resolve any mentioned issues. This feature is pretty essential for Haskell checking, in my opinion.
Stylistic issues were resolved and, I hope, no new were introduced.
The problem with
flycheck-fill-and-expand-error-file-namesnot having access to current working directory was fixed as well. The issue was critical for checkers that can report relative file names in their errors, but it didn't affect checkers that report absolute file names. So it was not as critical as last comment of #813 suggests, but it's fixed now anyway.To fix it, I added
default-directoryfield toflycheck-syntax-checkstructure and moved binding ofdefault-directoryvariable out offlycheck-start-command-checkerfunction into theflycheck-syntax-check-startfunction. This indirection obscures things a bit but allows to calculate default directory only once per cherker run. Since computing default directory may involve costly traversals of filesystem, it seems worth some additional obscurity.I regret not adding any new test cases, but it seems to require some non-trivial effort - the core repository seems to contain only, well, core tests and no tests for specific checkers.
Edit (@lunaryorn): Dear Waffle, this connects to #312.