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

Add a bunch of warnings flags, expose an Internal module #99

Merged
merged 4 commits into from Sep 29, 2022

Conversation

lf-
Copy link
Contributor

@lf- lf- commented Sep 29, 2022

The internal module is necessary to allow extending the library, and the warnings improve reliability.

@lf- lf- requested a review from friedbrice September 29, 2022 19:30
Comment on lines -63 to -69
#if MIN_VERSION_servant(0,16,0)
import Servant.Client.Core (AuthClientData, AuthenticatedRequest, Request, mkAuthenticatedRequest, addHeader)
#else
import Servant.Client.Core.Internal.Auth
import Servant.Client.Core (Request, addHeader)
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dropped the old versions since I don't sincerely believe that we are likely to correctly support them.

@@ -369,50 +328,6 @@ apiTest_
=
client (Proxy :: Proxy Api)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code block was moved to Web.Slack.Internal

Comment on lines +35 to +36
-- Requires explicit imports of _every_ function (e.g. '$'); too strict
-Wno-missing-import-lists

Choose a reason for hiding this comment

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

In Purescript they thought about this. They allow you to have exactly zero or one unqualified import that doesn't have an explicit import list 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clever!

I just copied the Mercury list :p

Comment on lines +120 to +121
, mono-traversable
, classy-prelude
Copy link

Choose a reason for hiding this comment

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

i really hate these libraries <.<

@@ -1,7 +1,6 @@
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE CPP #-}

Choose a reason for hiding this comment

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

Yay!

Comment on lines 1 to 3
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeFamilies #-}

Choose a reason for hiding this comment

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

I think hlint will tell you when a file has redundant language extensions. I'm not sure if it takes your `default-extensions into account.

Suggested change
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeFamilies #-}

Copy link

@friedbrice friedbrice left a comment

Choose a reason for hiding this comment

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

Looks great. Sorry I took a while, kept getting pulled away.

@lf- lf- merged commit 50e469c into master Sep 29, 2022
@lf- lf- deleted the jadel/mercury-standards branch September 29, 2022 22:43
lf- pushed a commit that referenced this pull request Sep 29, 2022
This is based on #99.

This code is copied (with approval) from the Mercury codebase, with no
code modifications except for making `SlackActionId` a `newtype` rather
than a sum type.
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.

None yet

2 participants