-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
Complete contributing guide #2165
Conversation
jneira
commented
Sep 7, 2021
•
edited by gitpod-io
bot
Loading
edited by gitpod-io
bot
- with the contents of ./CONTRIBUTING.md (mainly the pre commit hook to format)
With the contents of ./CONTRIBUTING.md (mainly the pre commit hook to format)
Included in the main documentation
As building and testing are more important imo
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.
Thank you for the update!
"hooks": [ | ||
{ | ||
"entry": "stylish-haskell --inplace", | ||
"exclude": "(^Setup.hs$|test/testdata/.*$|test/data/.*$|^hie-compat/.*$|^plugins/hls-tactics-plugin/.*$)", |
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.
Note: we need to update this regex to include up-to-date "excludes" setting in nix config. The current Nix config ignores more files than these.
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.
"exclude": "(^Setup.hs$|test/testdata/.*$|test/data/.*$|^hie-compat/.*$|^plugins/hls-tactics-plugin/.*$)", | |
"exclude": "(^Setup.hs$|test/testdata/.*$|test/data/.*$|test/manual/lhs/.*$|^hie-compat/.*$|^plugins/hls-tactics-plugin/.*$|^ghcide/src/Development/IDE/GHC/Compat.hs$|^plugins/hls-splice-plugin/src/Ide/Plugin/Splice.hs$|^ghcide/test/exe/Main.hs$|ghcide/src/Development/IDE/Core/Rules.hs|^hls-test-utils/src/Test/Hls/Util.hs$)", |
that would be correct?
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.
wow i did not apply this commit 🤦
Co-authored-by: Jan Hrcek <2716069+jhrcek@users.noreply.github.com>
Co-Authored-By: jhrcek
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.
Thank you. LGTM
An alternative, which only recompiles when tests (or dependencies) change: | ||
|
||
```bash | ||
$ cabal run haskell-language-server:func-test -- -p "hlint enables" |
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 don't think this works. It will use the HLS binary in your path, not the development build
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 seems this can make it not use the PATH one:
haskell-language-server/haskell-language-server.cabal
Lines 417 to 419 in 155023f
build-tool-depends: | |
haskell-language-server:haskell-language-server -any, | |
ghcide:ghcide-test-preprocessor -any |
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.
No, that won't help. cabal run
does not change the environment, as far as I know.
You can do cabal exec cabal run h-l-s:func-test -- -- -p ...
and it will work sometimes
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.
No, that won't help.
cabal run
does not change the environment, as far as I know.
It seems so: haskell/cabal#5411 (comment). It works for cabal test
but no for cabal run
iiuic that comment
You can do
cabal exec cabal run h-l-s:func-test -- -- -p ...
and it will work sometimes
It works enough times to update the instructions? are you are referring to the fact cabal exec
will not rebuild the executable and you could end using a older version?
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.
Moreover with cabal-install-3.8, test arguments will not trigger a rebuild
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 believe cabal exec
does trigger a rebuild, but it fails with weird errors in some cases.