-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
[#361] Put all warnings under if
s to always have them all
#409
Conversation
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.
Do you know why your PR is still not approved? Because I chose not to approve it. But they will.
21e02d4
to
0effe8c
Compare
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.
Cool stuff! Excited about such useful changes ❣️
cabal v2-build all | ||
cd "$TRAVIS_BUILD_DIR/summoner-cli/test/golden/fullProject/" | ||
echo "packages: ." > cabal.project |
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.
Clever fix!
-Wmissing-deriving-strategies (GHC >= 8.8.1) | ||
-Wincomplete-uni-patterns | ||
-Wincomplete-record-updates | ||
-Wredundant-constraints (GHC ⩾ 8.0) |
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.
niiice 😼 💅
CHANGELOG.md
Outdated
@@ -25,6 +25,9 @@ The changelog is available [on GitHub][2]. | |||
* [#363](https://github.com/kowainik/summoner/issues/363): | |||
Move from `generic-deriving` to `generic-data`. | |||
(by [@chshersh](https://github.com/chshersh)) | |||
* [#361](https://github.com/kowainik/summoner/issues/361): | |||
Always put all warnings in `ghc-options` inside common stanza under `if`s. |
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.
Let's add an example of if
in here if you don't mind 🙂
Also, maybe makes sense to mention that the condition is on GHC in these if
s? Or only one of these two options is fine as well, what do you think?
-- | As it will be shown in the @cabal@ file. | ||
cabalLicense :: LicenseName -> Text | ||
cabalLicense None = "AllRightsReserved" | ||
cabalLicense l = show l | ||
|
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.
That is a nice simplification!
ghcOptions = case settingsGhcOptions of | ||
[] -> defaultGhcOptions | ||
x:xs -> | ||
let customGhcOptions = T.intercalate "\n" $ x : map (T.replicate 21 " " <>) xs in |
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.
Could intercalateMap
function be handy in here?
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.
Yes, it can help here indeed! I missed that
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.
Or not.. looking into it I see, that the logic here is a bit different. Sorry, false alarm!
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.
Great!
Resolves #361