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

[#361] Put all warnings under ifs to always have them all #409

Merged
merged 2 commits into from
Feb 21, 2020

Conversation

chshersh
Copy link
Contributor

Resolves #361

@chshersh chshersh added generated project Files, folder generation by the summoner CI CI tools support; project CI; build with different GHC, tools refactoring docs Haddock, README, tutorials etc. labels Feb 20, 2020
@chshersh chshersh added this to the v2.0: Major update milestone Feb 20, 2020
@chshersh chshersh requested a review from vrom911 as a code owner February 20, 2020 20:32
@chshersh chshersh self-assigned this Feb 20, 2020
Copy link
Contributor

@hint-man hint-man bot left a 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.

summoner-cli/test/golden/smallProject/smallProject.cabal Outdated Show resolved Hide resolved
@chshersh chshersh force-pushed the chshersh/361-Put-all-warnings-under-ifs branch from 21e02d4 to 0effe8c Compare February 20, 2020 21:02
Copy link
Member

@vrom911 vrom911 left a 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
Copy link
Member

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)
Copy link
Member

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.
Copy link
Member

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 ifs? Or only one of these two options is fine as well, what do you think?

Comment on lines -68 to -72
-- | As it will be shown in the @cabal@ file.
cabalLicense :: LicenseName -> Text
cabalLicense None = "AllRightsReserved"
cabalLicense l = show l

Copy link
Member

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
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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!

Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Great!

@vrom911 vrom911 merged commit 3671a59 into master Feb 21, 2020
@vrom911 vrom911 deleted the chshersh/361-Put-all-warnings-under-ifs branch February 21, 2020 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI CI tools support; project CI; build with different GHC, tools docs Haddock, README, tutorials etc. generated project Files, folder generation by the summoner refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put all warnings under ifs to always have them all
2 participants