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

Support preprocessors #501

Open
eborden opened this issue Jun 20, 2018 · 9 comments
Open

Support preprocessors #501

eborden opened this issue Jun 20, 2018 · 9 comments

Comments

@eborden
Copy link
Contributor

eborden commented Jun 20, 2018

When building in CI hlint was unable to parse the README.lhs in this PR
freckle/faktory_worker_haskell#13 (comment)

This build utilizes the fpco docker build of lts-11.9, which contains https://www.stackage.org/lts-11.9/package/hlint-2.1.5

hlint .
Warning: unknown directive #faktory\_worker\_haskell
in ./README.lhs  at line 1 col 1
Warning: unknown directive ##
in ./README.lhs  at line 34 col 1
Warning: unknown directive ##
in ./README.lhs  at line 38 col 1
Warning: unknown directive ##
in ./README.lhs  at line 43 col 1
Warning: unknown directive ###
in ./README.lhs  at line 62 col 1
Warning: unknown directive ###
in ./README.lhs  at line 77 col 1
Warning: unknown directive ###
in ./README.lhs  at line 92 col 1
Warning: unknown directive ###
in ./README.lhs  at line 107 col 1
Warning: unknown directive ##
in ./README.lhs  at line 117 col 1
Warning: unknown directive ##
in ./README.lhs  at line 157 col 1
No hints

I did not have the same issues locally with:

$ hlint --version
HLint v2.0.10, (C) Neil Mitchell 2006-2017

However when I upgraded my local hlint I did see them.

$ hlint .
Warning: unknown directive #faktory\_worker\_haskell
in ./README.lhs  at line 1 col 1
Warning: unknown directive ##
in ./README.lhs  at line 34 col 1
Warning: unknown directive ##
in ./README.lhs  at line 38 col 1
Warning: unknown directive ##
in ./README.lhs  at line 43 col 1
Warning: unknown directive ###
in ./README.lhs  at line 63 col 1
Warning: unknown directive ###
in ./README.lhs  at line 78 col 1
Warning: unknown directive ###
in ./README.lhs  at line 93 col 1
Warning: unknown directive ###
in ./README.lhs  at line 108 col 1
Warning: unknown directive ##
in ./README.lhs  at line 118 col 1
Warning: unknown directive ##
in ./README.lhs  at line 158 col 1
No hints
@ndmitchell
Copy link
Owner

Those warnings are coming from cpphs - so if you pass --cpp-simple they might go away. However, it does seem that the file isn't a valid .lhs file - it is using markdown, not > ticks or \begin{code} which are the only valid LHS specs. I don't think you're going to get much benefit from running HLint on it.

@ndmitchell
Copy link
Owner

I added to the README at https://github.com/ndmitchell/hlint/blob/master/README.md#bugs-and-limitations:

HLint doesn't run any custom preprocessors, e.g. markdown-unlit or record-dot-preprocessor, so code making use of them will usually fail to parse.

That seems to be the crux of the issue. Should I leave this ticket to add support for preprocessors? Or do you think there is something HLint could do otherwise?

@eborden
Copy link
Contributor Author

eborden commented Jun 25, 2018

Yeah, I expected the issue to be preprocessors. If there is an expectation that hlint will add this support then there is likely value in keeping this open. Otherwise I'd just close it as a "won't fix" and consider the documentation a resolution.

@ndmitchell ndmitchell changed the title Failed to parse markdown-unlit Support preprocessors Jun 26, 2018
@ndmitchell
Copy link
Owner

Given I now have my own preprocessor the odds just went up significantly :)

@pkapustin
Copy link

I think it would be super awesome to add support for record-dot-preprocessor. Ghcide and Ormolu already support it, and I think it would be a great if HLint also did. I would definitely use this feature.

@ndmitchell
Copy link
Owner

My inclination is NOT to support preprocessors by default, but require them to be passed as a flag to HLint - so we wouldn't find the GHC preprocessor flags. Does that seem sufficient?

@pkapustin
Copy link

@ndmitchell It would be totally sufficient for me, I would simply like to be able to use HLint on a codebase that uses record-dot-preprocessor. Passing an extra flag to HLint is not a problem at all.

@ndmitchell
Copy link
Owner

For record-dot-preprocessor, is the problem you are getting parse errors (from a{b.c = d}? Or something else?), or that you are getting incorrect hints based on failing to group things properly (e.g. f a.b gets bracketed the wrong way?). Initial investigating suggests that actually running the preprocessor might harm the hints you get out of HLint, since it mangles the source code in ways that aren't so easy to understand after HLint takes a shot at it.

@pkapustin
Copy link

I just tested. Interestingly, there are no parse errors at all. I am getting correct hints (I am not sure whether any hints are missing due to incorrect grouping). Also, you are right, when I try to run HLint on the output of record-dot-preprocessor, the output is less useful as it contains too much stuff related to record-dot-preprocessor.

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

No branches or pull requests

3 participants