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 `flycheck-ghc-stack-project-file' variable for configuring stack project to build #1316

Merged
merged 1 commit into from Jan 23, 2018

Conversation

Projects
None yet
3 participants
@sergv
Contributor

sergv commented Sep 1, 2017

This allows users to run checks against custom stack yaml file that flycheck could never guess by itself. Guessing is hard because directory may contain several yaml files from which we'll need to pick
only one.

Fixes flycheck/flycheck-haskell#75.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 2, 2017

Is the value of the new variable intended to be a path, or just a single file? The documentation should probably state that explicitly.

Should :working-directory also uses the location of that overriding stack.yaml file?

Note: the CI fails because of a breaking change in buttercup, so we'll have to wait until we fix that before merging.

@sergv

This comment has been minimized.

Contributor

sergv commented Sep 3, 2017

The value should refer to just a single file. It cannot be directory because the aim is to be able to specify custom file name like stack-7.10.3.yaml which we can never guess on our own (because directory may contain multiple such files, for example). I'll update the docs.

The path can be either absolute, in which case :working-directory should have no effect, or relative, which should be resolved against :working-directory serving as root. I'll update the docs accordingly.

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from 161ef32 to 262781e Sep 3, 2017

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 7, 2017

The CI has been fixed, we can pick this up.

@sergv You'll need to document the option in doc/languages.rst.

@sergv

This comment has been minimized.

Contributor

sergv commented Sep 7, 2017

@fmdkdd I have updated doc/languages.rst file and rebased my branch.

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch 2 times, most recently from 9c0048d to 140b901 Sep 7, 2017

@fmdkdd

Thanks. I'm fine with the added option.

Could you add a test for it in flycheck-test.el?

@sergv

This comment has been minimized.

Contributor

sergv commented Sep 8, 2017

Given that there's no tests for other configuration variables to start with and that there's no tests for cabal/stack files, I'm not sure I can add the test quickly.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Sep 8, 2017

@sergv Does this fix need to be merged quickly? Or do you need a hand for writing the tests? I think the rust-cargo tests are good examples, since they use configuration variables to tests projects that also require a Cargo.toml file.

@sergv

This comment has been minimized.

Contributor

sergv commented Sep 8, 2017

@fmdkdd I think there's no rush to merge this change. I'll take a look at rust's tests - thanks for pointing them out!

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from 140b901 to 2693634 Oct 26, 2017

@sergv

This comment has been minimized.

Contributor

sergv commented Oct 26, 2017

@fmdkdd I have added a test for new variable.

@fmdkdd

Thanks for all the drive-by testing ;)

This looks fine. I can't run the integration test right now. @cpitclaudel Can you double check?

CHANGES.rst Outdated
@@ -9,6 +9,8 @@
- Add ``flycheck-cppcheck-suppressions-file`` to pass a suppressions
file to cppcheck [GH-1329]
- Add ``flycheck-ghc-stack-project-file`` option for ``haskell-stack-ghc``
checker for overriding default stack.yaml file.

This comment has been minimized.

@fmdkdd

fmdkdd Oct 26, 2017

Member

Please add the before default here.

flycheck.el Outdated
non-nil, stack will get overridden value via `--stack-yaml'."
:type 'string
:safe #'stringp
:package-version '(flycheck . "31"))

This comment has been minimized.

@fmdkdd

fmdkdd Oct 26, 2017

Member

The version number should be 32 now, not 31.

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from 2693634 to 6b2cf8e Oct 26, 2017

@sergv

This comment has been minimized.

Contributor

sergv commented Oct 26, 2017

Rebased and updated.

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from 6b2cf8e to dfa295c Nov 7, 2017

@cpitclaudel

Thanks. Some minor comments

-- |
-- Module : Foo
-- Copyright : (c) Sergey Vinokurov 2017
-- License : BSD3-style (see LICENSE)

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 20, 2017

Member

There is no LICENSE file in this folder, is there?

(proj-dir "language/haskell/stack-project-with-renamed-stack-yaml")
(flycheck-ghc-stack-project-file
(concat (flycheck-ert-resource-filename proj-dir)
"/stack-nonstandard.yaml")))

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 20, 2017

Member

expand-file-name is generally better than concat

flycheck.el Outdated
The value of this variable is either absolute or relative
filepath thar refers to yaml file for a stack project. Relative
filepath will be resolved against `:working-directory'. When
non-nil, stack will get overridden value via `--stack-yaml'."

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 20, 2017

Member

Wording suggestion:

The value of this variable is a file path that refers to a yaml 
file for the current stack project. Relative file paths are resolved
against the checker's working directory.
@@ -561,6 +561,11 @@ to view the docstring of the syntax checker. Likewise, you may use
Whether to enable Nix support for Stack (only for `haskell-stack-ghc`).
.. defcustom:: flycheck-ghc-stack-project-file
Allows to override default ``stack.yaml`` file for Stack, via

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 20, 2017

Member

"the default"

CHANGES.rst Outdated
@@ -10,6 +10,8 @@
- Add ``flycheck-cppcheck-suppressions-file`` to pass a suppressions
file to cppcheck [GH-1329]
- Add ``--force-exclusion`` flag to ``rubocop`` command [GH-1348]
- Add ``flycheck-ghc-stack-project-file`` option for ``haskell-stack-ghc``

This comment has been minimized.

@cpitclaudel

cpitclaudel Nov 20, 2017

Member

Add flycheck-ghc-stack-project-file for the haskell-stack-ghc checker

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from dfa295c to d6f2e93 Nov 20, 2017

@sergv

This comment has been minimized.

Contributor

sergv commented Nov 20, 2017

Thanks for minor comments. I have addressed them by amending commits and rebased the branch.

@fmdkdd fmdkdd added this to Backlog in Maintenance Nov 20, 2017

@fmdkdd fmdkdd removed this from Backlog in Maintenance Dec 12, 2017

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from d6f2e93 to 08cd312 Jan 22, 2018

@sergv

This comment has been minimized.

Contributor

sergv commented Jan 22, 2018

@fmdkdd @cpitclaudel This change seems pretty simple and non-far-reaching, can we merge it please?

@fmdkdd

fmdkdd approved these changes Jan 23, 2018

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jan 23, 2018

LGTM. You just need to squash your commit and we're good :)

@sergv

This comment has been minimized.

Contributor

sergv commented Jan 23, 2018

So should I manually squash the commits and push force it into the branch? Or do I just use 'Squash and merge' button, which seems to be disabled in this repo?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jan 23, 2018

You'll have to squash and force push. This is usually safe, unless you have based other work on this branch.

Add `flycheck-ghc-stack-project-file' variable for configuring stack …
…project to build

This allows users to run checks against custom stack yaml file that
flycheck could never guess by itself. Guessing is hard because
directory may contain several yaml files from which we'll need to pick
only one.

@sergv sergv force-pushed the sergv:add-user-option-for-selecting-stack-yaml-file branch from 08cd312 to 2050e9b Jan 23, 2018

@sergv

This comment has been minimized.

Contributor

sergv commented Jan 23, 2018

Squashed and pushed with force.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jan 23, 2018

You should probably rebase on master for the CI to pass.

@fmdkdd fmdkdd merged commit ad6a29d into flycheck:master Jan 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jan 23, 2018

Thanks!

@sergv

This comment has been minimized.

Contributor

sergv commented Jan 23, 2018

Cheers!

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jan 23, 2018

Thanks @sergv ! And thanks @fmdkdd for reviewing and merging :)

@sergv sergv deleted the sergv:add-user-option-for-selecting-stack-yaml-file branch Feb 24, 2018

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