cabal
cabal copied to clipboard
Let `cabal init` remember chosen language within current session
Fixes #10096
- [x] Patches conform to the coding conventions.
- [x] Any changes that could be relevant to users have been recorded in the changelog.
- [x] The documentation has been updated, if necessary.
- [x] Manual QA notes have been included.
- [x] Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)
QA Notes
Call cabal init and choose a package build that will end up asking for multiple languages. The last language you choose it should be the default option the next time. This way the user will just need to confirm that the same language is wanted.
% cabal init
Warning: The package list for 'hackage.haskell.org' is 24 days old.
Run 'cabal update' to get the latest list of available packages.
What does the package build:
1) Library
* 2) Executable
3) Library and Executable
4) Test suite
Your choice? [default: Executable] 3
...
Choose a language for your library:
* 1) Haskell2010
2) Haskell98
3) GHC2021 (requires at least GHC 9.2)
4) Other (specify)
Your choice? [default: Haskell2010] 3
...
Choose a language for your executable:
1) Haskell2010
2) Haskell98
* 3) GHC2021 (requires at least GHC 9.2)
4) Other (specify)
Your choice? [default: GHC2021]
...
Choose a language for your test suite:
1) Haskell2010
2) Haskell98
* 3) GHC2021 (requires at least GHC 9.2)
4) Other (specify)
Your choice? [default: GHC2021]
On top of whether the approach of adding the notion of Session m make sense, I would have liked to add some tests. But unit tests run with PurePrompt which can't hold state without further changes I am less certain how to do them.
Doing unit test using IO instead of PurePrompt seems like it was something that was avoided, so I didn't add that.
Another alternative would have been to turn the whole Interactive into a ReaderT pattern. This PR is getting away without it by passing the state in the real IO. But is more like a workaround.
Let me know if there is anything code wise I should iterate before addressing the rest of the checklist.
A ReaderT IO holding an IORef is a common pattern in the Haskell world, as it avoids a number of problems with State/StateT including inability to share state across threads or callbacks.
But If we use ReaderT IO holding an IORef in order to write unit test we would also need to loose the current PurePrompt I think. Probably I'm missing something to make things work easier.
In order to add tests I think I would need to change the PurePrompt have not only a [String] as state but also the session information (ie: lastChosenLanguage :: Maybe String).
But maybe it would also make sense to change the Interactive to be a ReaderT and avoid the explicit newSession.
I implemented a solution with ReaderT IORef and was able to add test for PurePrompt.
Adding a ReaderT pushed for the introduction of a PromptIO which is now the instance of Interactive. ie. IO is no longer an interactive.
Something that bugs me is that it seems that Interactive is used in two scenarios. As the monad used in cabal init but also as a helper to perform user input. Before the introduction of session state these scenarios were indistinguishable but now they are not. The effect is that I needed to add some runPromptIO on the later scenario that needed a IO as monad (see InstallSymlink.promptRun and PackageFileMonitor.invalidatePackageRegFileMonitor). Another instance is that I would have preferred for the writeProject call to be able to happen in regular IO and not with Session.
I tried (and failed) to leave Interactive class as it was and make a class Interactive => Session that will add the session state. To avoid duplicated code I attempt to use MonadIO but ended with some overlapping instances problem.
If dropping instance Interactive IO is a problem I can try to make another attempt, but maybe is fine as is.
Definitely these changes can be squashed.
Thank you for working on it! I'm sorry we're in a release death march now and can't quite review it immediately. But we'll get there!
I'm not sure if the release march is still going or if it ended with the release of 3.12.1.0 (the release information in github does not match the one in Hackage, I'm not familiar with the process). Either way, let me know if I need to iterate on this PR when you have a chance :-)
Oh, thanks for the ping! The march is over, indeed. I hope to do a review this week!
I squashed and rebased the whole PR in the first commit and then addressed all the feedback in separate commits. Let's see how CI feels 🤞 .
The bot will merge it during the week. Thanks!
And thanks for the manual QA notes. We'll try to find volunteers to check it.
Oops. I just pushed some more cleanups. I can take them back if needed. But I noticed some things that can be improved and send the last two commits.
Thanks for the feedback!
Don't worry, the bot restarts the timer on PR activity.
Manual QA passes here.
@bcardiff no worries! That's why we have a bot: it merges after two days of inactivity to create some buffer for last changes.
@mergify refresh
refresh
✅ Pull request refreshed
Strange: Mergify is refusing to proceed because GitHub is claiming bootstrap.yml changed (or perhaps that Mergify was trying to change it?) Or maybe because updating the PR against master would have changed it?
On a different note: how do I switch the status of project Manual QA board from Waiting to Testing Approved? This is baffling but I can't find a way. It used to be possible to drag and drop those cards (although I'd prefer to do that without visiting the project page), but this seems to have gone with the old GH projects?