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

Clean up more lint #10

Merged
merged 2 commits into from
Oct 29, 2016
Merged

Clean up more lint #10

merged 2 commits into from
Oct 29, 2016

Conversation

3noch
Copy link
Contributor

@3noch 3noch commented Oct 28, 2016

Some more changes that my linter (hlint) was complaining about but feel free to close if you're not a fan. I sort of wonder: do we really need Monad m constraints in all these places? Could Applicative suffice?

@ivan-m
Copy link
Owner

ivan-m commented Oct 29, 2016

I'm happy with Applicative + fmap instead of Monad + liftM if you want to make those changes. Just need to put a minimum bound on base for that then.

@@ -96,24 +96,24 @@ instance Monad m => IsString (m Doc) where
-- horizontally if that fits the page. Otherwise they are aligned
-- vertically. All comma separators are put in front of the
-- elements.
list :: (Monad m) => m [Doc] -> m Doc
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer parentheses even for singleton constraints (so they don't have to get added when another constraint is added).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll add those back and do the other changes.

@3noch
Copy link
Contributor Author

3noch commented Oct 29, 2016

So if we go through with this, the entire module is now incorrectly named! This is definitely a backwards breaking change as well.

@ivan-m
Copy link
Owner

ivan-m commented Oct 29, 2016

I'm happy with it still being named "Monadic" (at least for backwards compatability reasons); the main reason I wrote this was for my own usage of having a Stateful pretty-printer and that still operates/requires monadic aspects.

@3noch
Copy link
Contributor Author

3noch commented Oct 29, 2016

How do you feel about using base-compat's Prelude.Compat in the Monadic module instead of changing the bound on base?

@ivan-m
Copy link
Owner

ivan-m commented Oct 29, 2016

Sure, I'm happy with that.

@3noch
Copy link
Contributor Author

3noch commented Oct 29, 2016

Ok I've pushed changes. I used Functor instead of Applicative where possible. You may want to push our previous changes to Hackage before merging this since this will need to be a major version bump.

@ivan-m
Copy link
Owner

ivan-m commented Oct 29, 2016

I'm OK with a major version bump, especially as for anyone using a relatively recent version of GHC it's not going to be a problem (unless they were relying upon type coercion from the Monad constraint somehow).

@ivan-m ivan-m merged commit f866182 into ivan-m:master Oct 29, 2016
@3noch 3noch deleted the more-lint branch October 29, 2016 01:44
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.

2 participants