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.

Sign up for GitHub

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

Jump to bottom

Fix and improve list parser of cabal init cli #8663

Merged
merged 3 commits into from
Jan 23, 2023
Merged

Fix and improve list parser of cabal init cli #8663

merged 3 commits into from
Jan 23, 2023

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jan 11, 2023 •
edited
Loading

Fixes #8659

Fix cli list parser errors in cabal init

Occurrences of Flag [a] seem to behave in an unexpected way. The monoid
instance of Flag is right associative and discard the value on the
left.

Fixes for all Flag [a] the cli parser in cabal init. Adds tests for proving the improvement.

@fendor fendor changed the title WIP: Fix list parser of cabal init cli Fix list parser of cabal init cli Jan 11, 2023
@fendor fendor changed the title Fix list parser of cabal init cli Fix and improve list parser of cabal init cli Jan 11, 2023
@fendor
Copy link
Collaborator Author

fendor commented Jan 11, 2023

@emilypi Tests fail because tests use the state Flag [] to express something that I don't consider possible in the interactive and non-interactive execution. Namely, it uses Flag [] to avoid an entry in _libDependencies, allowing the following to hold: _libDependencies @?= []. However, when we use [], we now add the base dependency, forcing the assertion to become _libDependencies @?= [Dependency (PackageName "base") (MajorBoundVersion (mkVersion [4,16,4,0])) (fromNonEmpty (LMainLibName :| []))].

I think the new assertion is the more correct form since it is actually what's going to be generated. Any opinions?

@fendor fendor force-pushed the fix/cabal-init-cli-lists branch from 1c97965 to ff014da Compare January 15, 2023 16:46
@fendor
Fix cli list parse errors in cabal init
a93a7cd 
Occurrences of `Flag [a]` behave in a slightly unexpected way. The monoid
instance of `Flag` is right associative and discard the value on the
left.
Thus, make sure we merge the contents of the flags, instead of using the
monoid instance of `Flag` itself.
@fendor fendor force-pushed the fix/cabal-init-cli-lists branch from ff014da to a93a7cd Compare January 15, 2023 16:48
@fendor fendor requested a review from Mikolaj January 15, 2023 16:49
@fendor
Copy link
Collaborator Author

fendor commented Jan 15, 2023

Ready for review, just fix the parsers, and add test-cases for all modified parsers.

emilypi
emilypi approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

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

LGTM thanks @fendor

Mikolaj
Mikolaj requested changes Jan 16, 2023
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thanks a lot. I'm marking the absence of a changelog so that we don't miss it when reviewing again.

@fendor fendor force-pushed the fix/cabal-init-cli-lists branch from 3fa83f9 to a407d1c Compare January 16, 2023 12:02
@fendor fendor requested a review from Mikolaj January 16, 2023 12:02
Mikolaj
Mikolaj approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Brilliant.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 21, 2023

@fendor: when you are ready, feel free to set a label (I guess squash+merge_me?).

@fendor fendor added the squash+merge me Tell Mergify Bot to squash-merge label Jan 22, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Jan 23, 2023

This is the only PR left before cutting 3.10, so let me speed it up...

@Mikolaj Mikolaj added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 23, 2023
@mergify mergify bot merged commit 8aad429 into haskell:master Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilypi emilypi emilypi approved these changes

@Mikolaj Mikolaj Mikolaj approved these changes

Assignees
No one assigned
Labels
cabal-install: cmd/init merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days re: options Concerning command-line options squash+merge me Tell Mergify Bot to squash-merge type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cabal init doesn't honour multiple --dependency flags
3 participants
@fendor @Mikolaj @emilypi