Minor Cleanup #194
Minor Cleanup #194
Conversation
|
updated description. |
|
success! :-) |
src/Data/Swagger/Declare.hs
Outdated
| @@ -162,10 +162,6 @@ instance MonadDeclare d m => MonadDeclare d (IdentityT m) where | |||
| declare = lift . declare | |||
| look = lift look | |||
|
|
|||
| instance MonadDeclare d m => MonadDeclare d (ListT m) where | |||
There was a problem hiding this comment.
it created a deprecation warning:
Deprecated: This transformer is invalid on most monads
I figured anybody who wants to use it can still re-create the instance, but they will be prompted to think about it at least.
I'm happy to revert that change if you disagree?
There was a problem hiding this comment.
"re-create the instance" would mean an orphan one. No go.
There was a problem hiding this comment.
but providing an instance in this library for a transformer that will almost always lead to broken monads is ok? i disagree, but ok: cf0c2a7
| @@ -33,8 +34,9 @@ import Data.HashMap.Strict (HashMap) | |||
| import qualified Data.HashMap.Strict as HashMap | |||
| import qualified Data.HashMap.Strict.InsOrd as InsOrdHashMap | |||
| import qualified "unordered-containers" Data.HashSet as HashSet | |||
| import Data.List (intercalate) | |||
| #if __GLASGOW_HASKELL__ < 840 | |||
There was a problem hiding this comment.
we depend on base-compat-batteries, so
import Prelude ()
import Prelude.Compatand remeoving import Data.Monoid will solve this without CPP
| @@ -8,6 +8,7 @@ | |||
| {-# LANGUAGE GeneralizedNewtypeDeriving #-} | |||
| {-# LANGUAGE LambdaCase #-} | |||
| {-# LANGUAGE MultiParamTypeClasses #-} | |||
| {-# LANGUAGE NoImplicitPrelude #-} | |||
There was a problem hiding this comment.
We don't use NoImplicitPrelude. I have no strong opinion about it specifically, but I value consistency, and elsewhere we use Prelude.Compat and Prelude imports as own block separated from other imports. For example
module Data.Swagger.Internal where
import Prelude ()
import Prelude.Compat
import Control.Lens ((&), (.~), (?~))
import Control.Applicative
import Data.Aeson
...There was a problem hiding this comment.
Thanks for bearing with me... :)
I hope I got it right this time: 5e71133
Even realized I should turn off CPP again if I don't use it...
|
@phadej have I done everything you requested? |
|
just a friendly reminder that i'm still interested in this. @phadej? or perhaps somebody else has the authority to decide that i've answered all his requests? |
|
Please let me know if I should
If I don't hear anything within 5 days, I will take the one approving review seriously, assume it's ready to go to master, and squash-merge (my preference). |
|
I've cleaned up the branch, and there is not much left. I'll merge it together with #195 on Oct 22 or after if nobody stops me. |
New test case. Seems trivial now, but it took me a while (too long) to realize it's the expected behavior.
Update: I also removed some trivial warnings, and one about
ListTthat is not quite trivial. I'm happy to drop that commit that if you disagree.Update: I removed the test case because it made no sense. It's probably a good idea to squash-merge this. Let me know if you want me to rebase instead.