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

feat: instance MonadLogic for WriterT #33

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

ncaq
Copy link
Contributor

@ncaq ncaq commented Dec 23, 2022

I have implemented MonadLogic in WriterT as well.

Until now, WriterT should not be used because it easily causes space leaks. However, Control.Monad.Trans.Writer.CPS no longer causes space leaks and can be used. Therefore, we would like to use WriterT, but it is inconvenient if it cannot be combined with MonadLogic.

Therefore, I implemented it and added tests where ReaderT and StateT are tested.

To use the CPS version of WriterT, I raised the mtl version requirement to feat(Control.Monad.Writer.CPS): re export runWriterT by ncaq · Pull Request #136 · haskell/mtl has not yet been imported, so we have no choice but to rely directly on transformers. In the end, mtl depends on transformers, so we have determined that this is not a critical problem. We have taken care to make it easy to remove them when they are no longer needed by doing a limited import.

I was surprised myself that many of the CPP macros in the test code were removed. It was not my intention.
I believe it was probably done by the HLS plugin.
I've been trying to find out why it deleted them. I found out that the new mtl raises the lower limit for the base library in the cabal file. So when raising the lower version limit of mtl, we thought that it is certainly not a very effective practice to control the base version here. Therefore, we decided to leave the deleted part as it is.

@ncaq
Copy link
Contributor Author

ncaq commented Dec 23, 2022

Old GHCs were removed from cabal files and CI as a result of the mtl version upgrade.
We have decided that since mtl has removed support, we have no choice.
For security reasons, it seems that CI won't work without approval.

@Bodigrim
Copy link
Owner

We have decided that since mtl has removed support, we have no choice.

May I ask who are "we" making decisions here?

I appreciate your contribution, but please wrap the new instance (and tests) into #if MIN_VERSION_mtl(2,3,0) ... #endif so that logict can continue to support older releases of GHC / mtl.

@ncaq
Copy link
Contributor Author

ncaq commented Dec 24, 2022

Sorry, I am typing the text via machine translation DeepL so the first person is not stable.
I did not intend to pressure you to use we as if it were a consensus.
I should have used I in this case.

I understand that logict's policy is to try to support older GHCs as well.
I was getting ahead of myself in thinking that the scope of support would be as good as mtl.

I will make the correction as you suggest next time I get time to work on it.

@ncaq
Copy link
Contributor Author

ncaq commented Jan 13, 2023

I tested cabal test and cabal test --constraint='mtl >= 2.3' but I don't know how to run GitHub Actions on my account.

@ncaq ncaq changed the title feat!: instance MonadLogic for WriterT feat: instance MonadLogic for WriterT Jan 13, 2023
@Bodigrim
Copy link
Owner

FWIW I'll accept a (separate) PR, dropping support of GHC < 8.0.

@ncaq
Copy link
Contributor Author

ncaq commented Jan 16, 2023

I'll try some more...

@ncaq
Copy link
Contributor Author

ncaq commented Jan 16, 2023

Maybe, It's OK.
fix: nats support GHC 7 · ncaq/logict@dfd5085

@Bodigrim
Copy link
Owner

Thanks, LGTM. Could you please rebase?

I have implemented `MonadLogic` in `WriterT` as well.

Until now, `WriterT` should not be used because it easily causes space leaks.
However, [Control.Monad.Trans.Writer.CPS](https://www.stackage.org/haddock/nightly-2022-12-08/transformers-0.5.6.2/Control-Monad-Trans-Writer-CPS.html) no longer causes space leaks and can be used.
Therefore, we would like to use `WriterT`, but it is inconvenient if it cannot be combined with `MonadLogic`.

Therefore, I implemented it and added tests where `ReaderT` and `StateT` are tested.

To use the CPS version of `WriterT`, I raised the mtl version requirement to [feat(Control.Monad.Writer.CPS): re export runWriterT by ncaq · Pull Request #136 · haskell/mtl](haskell/mtl#136) has not yet been imported, so we have no choice but to rely directly on transformers.
In the end, mtl depends on transformers, so we have determined that this is not a critical problem.
We have taken care to make it easy to remove them when they are no longer needed by doing a limited `import`.
@ncaq ncaq force-pushed the add-MonadLogic-for-Writer branch from dfd5085 to 10dbf8f Compare January 18, 2023 09:08
@ncaq
Copy link
Contributor Author

ncaq commented Jan 18, 2023

I did a git rebase.

@Bodigrim Bodigrim merged commit 754249f into Bodigrim:master Jan 18, 2023
@Bodigrim
Copy link
Owner

Great, thanks @ncaq!

@ncaq ncaq deleted the add-MonadLogic-for-Writer branch January 19, 2023 12:20
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