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.

Sign up for GitHub

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

Don’t let MonadZero imply its deprecation warning #68

Merged
merged 4 commits into from
Dec 8, 2020
Merged

Don’t let MonadZero imply its deprecation warning #68

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aac34f3
Don’t let MonadZero imply its deprecation warning
kl0tl Dec 6, 2020
1ef1d12
Rename Deprecated to DeprecatedMonadZero
JordanMartinez Dec 7, 2020
9fc35f0
Rename DeprecatedMonadZero to MonadZeroIsDeprecated
JordanMartinez Dec 8, 2020
34b5229
Merge branch 'master' into no-warn-superclass
thomashoneyman Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/Control/MonadZero.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
-- | Use `Monad` and `Alternative` constraints instead.

module Control.MonadZero
( class MonadZero
( class Deprecated
, class MonadZero
, module Control.Alt
, module Control.Alternative
, module Control.Applicative
Expand All @@ -32,13 +33,16 @@ import Data.Functor (class Functor, map, void, ($>), (<#>), (<$), (<$>))

import Prim.TypeError (class Warn, Text)

class Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Will this work if we have multiple items deprecated? Perhaps we could use a pattern like ‘DeprecatedMonadZero’ if we need to distinguish classes in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that making this class name more specific could help other issues in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've renamed the class. Waiting on CI to build.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure to understand why we should rename the class actually. Don’t the core libraries encourage to qualify imports to disambiguate?

Copy link
Member

@thomashoneyman thomashoneyman Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I meant for within a single module when there are multiple deprecations; here it doesn’t really matter as far as errors go. Though I think it might help make the class more obviously not a generic deprecated class if we name it more specifically for the function or class which is deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, I didn’t pay attention to your first sentence initially for some reason 🤦

What do you think of an IsDeprecated suffix instead? I’m ok with whatever pattern you prefer though.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine as well -- just something more specific than Deprecated. So MonadZeroIsDeprecated is great, DeprecatedMonadZero is fine, whatever works!

Copy link
Contributor

Choose a reason for hiding this comment

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

I like MonadZeroIsDeprecated because it's clearer than DeprecatedMonadZero. The former reads like a statement whereas the latter is left to interpretation.

instance deprecated :: Warn (Text "'MonadZero' is deprecated, use 'Monad' and 'Alternative' constraints instead") => Deprecated

-- | The `MonadZero` type class has no members of its own; it just specifies
-- | that the type has both `Monad` and `Alternative` instances.
-- |
-- | Types which have `MonadZero` instances should also satisfy the following
-- | laws:
-- |
-- | - Annihilation: `empty >>= f = empty`
class (Monad m, Alternative m, Warn (Text "'MonadZero' is deprecated, use 'Monad' and 'Alternative' constraints instead")) <= MonadZero m
class (Monad m, Alternative m, Deprecated) <= MonadZero m

instance monadZeroArray :: MonadZero Array