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

Fix almost all warnings #451

Merged
merged 6 commits into from
Jan 30, 2018
Merged

Fix almost all warnings #451

merged 6 commits into from
Jan 30, 2018

Conversation

alicebob
Copy link
Contributor

@alicebob alicebob commented Jan 26, 2018

Most should be fine, but the last two commits might be too big a diff. Up to you what you do with this.

There are a few warnings left in a CLI module, but that one seems used only in tests (I'm not sure how to fix them).

@alicebob
Copy link
Contributor Author

For #302

@@ -5,82 +5,82 @@ import Text.Parsec ((<|>), (<?>), char, many1, string, try, optionMaybe)

import Parse.Helpers
import qualified Reporting.Annotation as A
import AST.V0_16
import qualified AST.V0_16 as AST
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did qualifying this help fix warnings, or was this just an additional cleanup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for these 4 warnings:

[53 of 61] Compiling Parse.Type       ( parser/src/Parse/Type.hs, .stack-work/dist/x86_64-linux-nopie/Cabal-1.24.2.0/build/Parse/Type.o )

.../elm-format/parser/src/Parse/Type.hs:41:13: warning: [-Wname-shadowing]
    This binding for ‘base’ shadows the existing binding
      imported from ‘AST.V0_16’ at parser/src/Parse/Type.hs:8:1-16

.../elm-format/parser/src/Parse/Type.hs:42:14: warning: [-Wname-shadowing]
    This binding for ‘fields’ shadows the existing binding
      imported from ‘AST.V0_16’ at parser/src/Parse/Type.hs:8:1-16

.../elm-format/parser/src/Parse/Type.hs:91:24: warning: [-Wname-shadowing]
    This binding for ‘first’ shadows the existing binding
      imported from ‘AST.V0_16’ at parser/src/Parse/Type.hs:8:1-16

.../elm-format/parser/src/Parse/Type.hs:91:31: warning: [-Wname-shadowing]
    This binding for ‘rest’ shadows the existing binding
      imported from ‘AST.V0_16’ at parser/src/Parse/Type.hs:8:1-16

Since thse names of the exported methods from AST.V0_16 are so generic I choose to solve it with qualifying the import. Same reasoning for the other file in this PR where AST.V0_16 is imported.

@@ -49,6 +49,8 @@ showErrorMessage OutputAndValidate =
showErrorMessage (MustSpecifyVersionWithUpgrade elmVersion) =
"I can only upgrade code to the latest Elm version. To make sure I'm doing what you expect, you must also specify --elm-version=" ++ show elmVersion ++ " when you use --upgrade."

showErrorMessage NoInputs =
"No inputs"
Copy link
Owner

@avh4 avh4 Jan 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Why did this not lead to crashes before? Is NoInputs actually used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/ElmFormat.hs:

                    case (elmVersionResult, determineWhatToDoFromConfig config resolvedInputFiles) of
                        (_, Left NoInputs) ->
                            (handleParseResult $ Flags.showHelpText defaultVersion elmFormatVersion experimental)
                                >> exitFailure

                        (_, Left message) ->
                            exitWithError message
...

exitWithError uses showErrorMessage (which is what the warning is about). So the NoInputs case is effectively impossible because it is handled in a special way.

@avh4 avh4 merged commit 45c8e6b into avh4:master Jan 30, 2018
@avh4
Copy link
Owner

avh4 commented Jan 30, 2018

Great, thanks! I added 45c8e6b...101460c

@avh4 avh4 added this to the 0.7.1 public beta milestone Jan 30, 2018
@alicebob alicebob deleted the warnings branch January 30, 2018 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants