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

Add current working directory [:default-directory] property #973

Merged
merged 16 commits into from
Jul 7, 2016
Merged

Add current working directory [:default-directory] property #973

merged 16 commits into from
Jul 7, 2016

Conversation

sergv
Copy link
Contributor

@sergv sergv commented May 17, 2016

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-names not 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-directory field to flycheck-syntax-check structure and moved binding of default-directory variable out of flycheck-start-command-checker function into the flycheck-syntax-check-start function. 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.

@swsnr
Copy link
Contributor

swsnr commented May 18, 2016

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

@sergv
Copy link
Contributor Author

sergv commented May 22, 2016

I feel uneasy for hijacking this pull request with fix for #978, but the fix is slightly dependent on changes that introduced cwd argument in the flycheck-fix-error-filename function.

@swsnr
Copy link
Contributor

swsnr commented May 23, 2016

@sergv Please remove that change from this PR. It's unrelated, and while I'm all in favour of a default-directory, I'm not convinced that replacing file names in error messages is a good idea.

Please let's discuss this separately, even if this means that the fix for #978 will be delayed until after this PR is merged.

@sergv
Copy link
Contributor Author

sergv commented May 23, 2016

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
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a spurious whitespace change?

Copy link
Contributor Author

@sergv sergv Jun 2, 2016

Choose a reason for hiding this comment

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

Good catch but actually it isn't a whitespace change - the inline list gets compared against errors list, which has 4 entries now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh dear, I should have taken a better look. Sorry for the noise 😊

@swsnr
Copy link
Contributor

swsnr commented Jun 5, 2016

@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 default-directory was only part of flycheck-define-command-checker, and stored in the process object returned by this function. I think it'd be easy enough to attach the working directory with (process-put process 'flycheck-default-directory default-directory), and later use process-get to extract it.

Could you try to refactor the PR accordingly?

@sergv
Copy link
Contributor Author

sergv commented Jun 5, 2016

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

@swsnr
Copy link
Contributor

swsnr commented Jun 5, 2016

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

@sergv
Copy link
Contributor Author

sergv commented Jun 8, 2016

@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 git diff 99c26f6..cwd^, for example. In addition, I moved default-directory setting back into checker process object and disallowed non-function values of :default-directory setting3.

@@ -4257,6 +4263,27 @@ universal prefix arg, and only the id with normal prefix arg."
(`(eval ,_) t)
(_ nil)))

(defun flycheck-default-directory-wrapper (checker)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

@sergv
Copy link
Contributor Author

sergv commented Jul 7, 2016

@lunaryorn now the button is green, thanks! I'll rebase and merge.

s3bs and others added 16 commits July 7, 2016 10:34
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.
@sergv
Copy link
Contributor Author

sergv commented Jul 7, 2016

@lunaryorn However, it seems new approval is needed after rebase. Can you please approve again?

@sergv sergv merged commit 472eb7c into flycheck:master Jul 7, 2016
@sergv
Copy link
Contributor Author

sergv commented Jul 7, 2016

@lunaryorn For some reason, github still created a merge commit. Not sure what to do about it.

@sergv sergv deleted the cwd branch July 7, 2016 07:45
@mrkkrp
Copy link
Contributor

mrkkrp commented Jul 7, 2016

Now that we have this merged, we can make ghc and ghc-stack be called as if from root directory of project, right? Is there any plans to do this change, or if you point where to look how this new feature works I can open a PR myself.

@swsnr
Copy link
Contributor

swsnr commented Jul 7, 2016

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

swsnr added a commit that referenced this pull request Jul 7, 2016
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
@swsnr
Copy link
Contributor

swsnr commented Jul 7, 2016

@sergv I've changed the interface of the :default-directory function. It now takes the syntax checker symbol as argument, to allow to reuse the same function for multiple checkers while optionally dispatching on the syntax checker, and it may return nil now to fall back to the normal default directory, for instance in case a project file isn't found. I think these changes are reasonable, do you agree?

Meanwhile, would you please take a look at #1005? This change seems to have broken generic syntax checkers.

@sergv
Copy link
Contributor Author

sergv commented Jul 7, 2016

@lunaryorn The change to :default-directory looks reasonable. I'll look at #1005.

@lunaryorn @mrkkrp Regarding default-directory property for haskell-stack-ghc checker, I can submit one. However this would probably mean that we should add :default-directory argument to flycheck-define-checker macro since haskell-stack-ghc is defined using it. Would you agree with such addition, @lunaryorn?

@swsnr
Copy link
Contributor

swsnr commented Jul 7, 2016

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

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

Successfully merging this pull request may close these issues.

None yet

4 participants