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

Hlint more partial functions, and Debug.Trace #3000

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

michaelpj
Copy link
Collaborator

@michaelpj michaelpj commented Jun 29, 2022

Turns out that for functions which are typically imported in multiple ways, you need to list both explicitly, and you can't combine them (see the linked HLint issue). So I did that and added some more exceptions.

I also banned Debug.Trace, removing a couple of dead usages and adding exceptions for the existing (!) usages.

@wz1000
Copy link
Collaborator

wz1000 commented Jun 29, 2022

Can we have global exceptions for traceEvent, traceEventIO, traceMarker and traceMarkerIO? These should cover some of the existing module exceptions.

@michaelpj
Copy link
Collaborator Author

Hmm, is eventlog tracing not exported from anywhere else? That's a shame.

@michaelpj
Copy link
Collaborator Author

I'm not sure if I can do that easily, I might have to switch to explicitly listing the banned functions from Debug.Trace.

@michaelpj
Copy link
Collaborator Author

I'm doing that, but in fact it doesn't let us get rid of any exceptions, since all of those modules also use non-eventlog tracing functions 🤷

@michaelpj michaelpj force-pushed the mpj/more-partial-function-hlint branch from 756f7d0 to e07a6fe Compare June 29, 2022 09:52
@wz1000
Copy link
Collaborator

wz1000 commented Jun 29, 2022

Maybe change those instances to use the existing log mechanism where it is easy? Perhaps a job for another time.

@michaelpj
Copy link
Collaborator Author

Yep, once the ratchet exists it's a nice TODO list for anyone who feels like it :)

@drsooch
Copy link
Collaborator

drsooch commented Jun 29, 2022

Is there no way to ignore whole directories? For example the test suites seem like it would be ok to allow certain partial functions

@michaelpj
Copy link
Collaborator Author

Not sure, I think it works off them module name anyway. I'm also not too bothered by it - the explicit list is not actually that long!

@drsooch
Copy link
Collaborator

drsooch commented Jul 1, 2022

Fair enough. Just looks tedious 😂

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Jul 6, 2022
@mergify mergify bot merged commit a32db0b into haskell:master Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants