cabal icon indicating copy to clipboard operation
cabal copied to clipboard

Let `cabal init` remember chosen language within current session

Open bcardiff opened this issue 1 year ago • 6 comments

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]

bcardiff avatar Jun 14 '24 13:06 bcardiff

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.

bcardiff avatar Jun 14 '24 19:06 bcardiff

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.

geekosaur avatar Jun 14 '24 19:06 geekosaur

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.

bcardiff avatar Jun 14 '24 20:06 bcardiff

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.

bcardiff avatar Jun 18 '24 13:06 bcardiff

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.

bcardiff avatar Jun 21 '24 01:06 bcardiff

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!

ulysses4ever avatar Jun 21 '24 01:06 ulysses4ever

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 :-)

bcardiff avatar Jul 11 '24 12:07 bcardiff

Oh, thanks for the ping! The march is over, indeed. I hope to do a review this week!

ulysses4ever avatar Jul 11 '24 12:07 ulysses4ever

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 🤞 .

bcardiff avatar Aug 18 '24 02:08 bcardiff

The bot will merge it during the week. Thanks!

ulysses4ever avatar Aug 18 '24 18:08 ulysses4ever

And thanks for the manual QA notes. We'll try to find volunteers to check it.

ulysses4ever avatar Aug 18 '24 18:08 ulysses4ever

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!

bcardiff avatar Aug 18 '24 18:08 bcardiff

Don't worry, the bot restarts the timer on PR activity.

geekosaur avatar Aug 18 '24 18:08 geekosaur

Manual QA passes here.

geekosaur avatar Aug 18 '24 18:08 geekosaur

@bcardiff no worries! That's why we have a bot: it merges after two days of inactivity to create some buffer for last changes.

ulysses4ever avatar Aug 18 '24 19:08 ulysses4ever

@mergify refresh

geekosaur avatar Aug 22 '24 00:08 geekosaur

refresh

✅ Pull request refreshed

mergify[bot] avatar Aug 22 '24 01:08 mergify[bot]

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?

geekosaur avatar Aug 22 '24 01:08 geekosaur

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?

ulysses4ever avatar Aug 22 '24 02:08 ulysses4ever