Open sjakobi opened 2 years ago
Phew, good question. The company I currently work for uses -Wall
as well, but I'm not sold on its effectiveness. Making sure that the code has no warnings does help with keeping the code tidy, but often at the cost of making it more work to write.
My opinions on specific warnings are:
-Wunused-do-bind
— This warning is actually not compatible with the "chain of actions" style that Threepenny-Gui uses, e.g. element x # set attr "foo" # …
; I deliberately want to ignore that one.-Wname-shadowing
— Sometimes, this does catch bugs, but the price is that it discourages short names at the top level. For example, path
at the top level will likely become getPath
in order to have path
available in local scopes. I tend to be in favor of ignoring that one.-Wunused-imports
— Without automatic import management by the IDE, this warning slows down development considerably. I tend to ignore it until just before merging a pull request.-Wincomplete-patterns
— This one is ok. Threepenny-GUI does use a lot of partial functions for marshaling, but avoiding them did catch a bug for me once. Adding a _ → error "some more informative message"
case is not too bad a solution.-Wmissing-signatures
— I like this one. Sometimes, adding a signature requires adding imports, but that's more of a problem when using explicit import lists (ugh). Of course, in special circumstances, such as the Attributes
module, we want to ignore it.-Wunused-matches
— I like this one. Adding a bar _
in front of a name does not hurt.If you like, feel free to turn on warnings about 3–6 and others that I missed.
-Wunused-do-bind
— This warning is actually not compatible with the "chain of actions" style that Threepenny-Gui uses, e.g.element x # set attr "foo" # …
; I deliberately want to ignore that one.
Yeah, we can turn this off with -Wno-unused-do-binds
in the .cabal
file.
2.
-Wname-shadowing
— Sometimes, this does catch bugs, but the price is that it discourages short names at the top level. For example,path
at the top level will likely becomegetPath
in order to havepath
available in local scopes. I tend to be in favor of ignoring that one.
How about renaming the local path
to path_
? I've seen this style in some places.
3.
-Wunused-imports
— Without automatic import management by the IDE, this warning slows down development considerably. I tend to ignore it until just before merging a pull request.
I think it can help remove clutter and discover unused dependencies. Note that you can of course temporarily use -Wno-unused-imports
for development once we've enabled -Wall
.
I agree with your points 4 to 6.
I'm not sure when I'll get to implementing this. I've marked this issue "good first issue" in case someone's interested.
No rush. 😄 Thanks for looking into this, Simon!
I just noticed that this package doesn't use
-Wall
in the library or in the executables. When I enable-Wall
for the library, I get (with GHC-9.2.1):Of course, not all of these warnings are equally interesting. But some probably are.
My inclination would be to enable
-Wall
, try to address most of the warnings, and maybe disable certain warnings on a per-file basis, e.g. use-Wno-missing-signatures
insrc/Graphics/UI/Threepenny/Attributes.hs
.What do you think, @HeinrichApfelmus?