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

Let cabal init remember chosen language within current session #10115

Merged
merged 13 commits into from
Aug 22, 2024

Conversation

bcardiff
Copy link
Contributor

@bcardiff bcardiff commented Jun 14, 2024

Fixes #10096

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • 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]

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Not blocking on this because it's arguable, and in particular my newTVarIO example isn't doing the same thing. But IIRC there are other examples, so it's worth consideration.

cabal-install/src/Distribution/Client/Init/Types.hs Outdated Show resolved Hide resolved
@bcardiff
Copy link
Contributor Author

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.

@geekosaur
Copy link
Collaborator

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.

@bcardiff
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@ulysses4ever
Copy link
Collaborator

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!

@bcardiff bcardiff changed the title WIP Let cabal init remember chosen language within current session Let cabal init remember chosen language within current session Jun 21, 2024
@bcardiff bcardiff marked this pull request as ready for review June 21, 2024 14:07
@bcardiff
Copy link
Contributor Author

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

@ulysses4ever
Copy link
Collaborator

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

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thank you! And sorry it took so long to get back to you.

cabal-install/src/Distribution/Client/Init.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Init/Types.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Init/Types.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Init/Types.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Init/Types.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Init/Types.hs Outdated Show resolved Hide resolved
@bcardiff bcardiff force-pushed the bcardiff/remember-chosen-language branch from 228ff16 to 6fbd20c Compare August 18, 2024 02:41
@bcardiff
Copy link
Contributor Author

bcardiff commented Aug 18, 2024

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

@ulysses4ever ulysses4ever added the squash+merge me Tell Mergify Bot to squash-merge label Aug 18, 2024
@ulysses4ever
Copy link
Collaborator

The bot will merge it during the week. Thanks!

@ulysses4ever
Copy link
Collaborator

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

@ulysses4ever ulysses4ever added the attention: needs-manual-qa PR is destined for manual QA label Aug 18, 2024
@bcardiff
Copy link
Contributor Author

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!

@geekosaur
Copy link
Collaborator

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

@geekosaur
Copy link
Collaborator

Manual QA passes here.

@ulysses4ever
Copy link
Collaborator

@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 mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Aug 20, 2024
@geekosaur
Copy link
Collaborator

@mergify refresh

Copy link
Contributor

mergify bot commented Aug 22, 2024

refresh

✅ Pull request refreshed

@geekosaur
Copy link
Collaborator

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?

@ulysses4ever
Copy link
Collaborator

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?

@mergify mergify bot merged commit a1c94c1 into haskell:master Aug 22, 2024
50 checks passed
@jasagredo jasagredo added the tested-on: windows QA has been successful on Windows label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-manual-qa PR is destined for manual QA merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge tested-on: windows QA has been successful on Windows
Projects
Status: To Test
Development

Successfully merging this pull request may close these issues.

Remember "Choose a language for" choice on cabal init
4 participants