Skip to content

Concat arguments when enabling haddock #10783

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
Merged
merged 3 commits into from
Mar 20, 2025
Merged

Concat arguments when enabling haddock #10783

merged 3 commits into from
Mar 20, 2025

Conversation

jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Feb 8, 2025 •
edited
Loading

Arguments were unioned with <> which is left-biased, therefore -haddock swallowed all other args as indeed described in #10782.

QA

The example in #10782 now correctly will fail even if documentation is enabled.

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

@jasagredo jasagredo force-pushed the js/haddock-swallows-other branch from 734f4f9 to 8e1f76b Compare February 8, 2025 15:48
@jasagredo
Copy link
Collaborator Author

I think the bug was introduced by 80f0a65, which makes the expression modified in this PR to be non-empty if documentation was enabled. I however don't know if I'm introducing some subtle bug by not ignoring the default args anymore but concatenating them with the user supplied args.

@jasagredo jasagredo force-pushed the js/haddock-swallows-other branch from 8e1f76b to ad84f9e Compare February 8, 2025 15:53
@jasagredo jasagredo marked this pull request as ready for review February 8, 2025 15:53
@jasagredo jasagredo linked an issue Feb 8, 2025 that may be closed by this pull request
Enabling documentation seems to swallow ghc-options passed #10782
Closed
@jasagredo jasagredo mentioned this pull request Feb 8, 2025
Closed
@jasagredo jasagredo force-pushed the js/haddock-swallows-other branch from ad84f9e to 3a43753 Compare February 9, 2025 10:17
@Mikolaj
Copy link
Member

Mikolaj commented Feb 12, 2025

@FinleyMcIlwaine, could you have quick look?

Mikolaj
Mikolaj approved these changes Feb 13, 2025
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.

LGTM

@ffaf1
Copy link
Collaborator

ffaf1 commented Feb 13, 2025

Seems ok to me, is there a way to add a test?

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2025

Ping, ping! This is an important fix, let's get it (tested and) merged. Maybe the original reproducer could be used as a test?

@jasagredo
Copy link
Collaborator Author

I think a test would be possible to add. I however don't have the time at the moment for doing cabal stuff in my free time.

@Kleidukos
Copy link
Member

@FinleyMcIlwaine double-ping! Can we have confirmation that this goes in the direction you intended in your last patch?

Kleidukos
Kleidukos requested changes Feb 27, 2025
View reviewed changes
Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

I'd like to see a test, if only for the concatenation logic.

@Kleidukos
Copy link
Member

Thanks a lot @jasagredo! :)

@mpickering
Copy link
Collaborator

I can add a test for this issue in the next week.

@FinleyMcIlwaine
Copy link
Contributor

Sorry for the delay here, this looks good to me! Nice catch

@Kleidukos
Copy link
Member

Thank you both. :)

jasagredo and others added 2 commits March 5, 2025 11:37
@jasagredo @mpickering
Concat arguments when enabling haddock
1c6fcb1 
Arguments were unioned with <> which is left-biased, therefore -haddock
swallowed all other args as indeed described in haskell#10782.

I think the bug was introduced by 80f0a65, which makes the expression
modified in this PR to be non-empty if documentation was enabled.

Fixes haskell#10782
@mpickering
Add test for haskell#10783 (concatenate arguments when enabling haddock)
899899f
@mpickering mpickering force-pushed the js/haddock-swallows-other branch from 42f263b to 899899f Compare March 5, 2025 11:39
@mpickering
Copy link
Collaborator

I have pushed onto your branch @jasagredo

  • I fleshed out the commit message of the original problem
  • Added a test which I checked fails before the patch and now succeeds. Thanks for the good reproducer @Swordlash

Kleidukos
Kleidukos approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@Kleidukos Kleidukos left a comment

Choose a reason for hiding this comment

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

Excellent, thank you very much!

@jasagredo
Copy link
Collaborator Author

Awesome @mpickering! Thank you very much for taking care of the PR

@Mikolaj
Copy link
Member

Mikolaj commented Mar 13, 2025

@jasagredo: is this ready? Could you set the merge label?

@Swordlash: can you confirm this fixes you issue?

@Mikolaj
Copy link
Member

Mikolaj commented Mar 17, 2025

@jasagredo seems busy, so let me set the label, given that users are clamoring for a bugfix release.

@Mikolaj Mikolaj added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Mar 17, 2025
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Mar 17, 2025
@ulysses4ever ulysses4ever added this to the cabal-install 3.14.2.1 milestone Mar 18, 2025
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Mar 20, 2025
@mergify mergify bot merged commit ef29ebb into haskell:master Mar 20, 2025
46 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

@mergify backport 3.14

@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 28, 2025 •
edited
Loading

backport 3.14

✅ Backports have been created

mergify bot added a commit that referenced this pull request Mar 28, 2025
@jasagredo @mpickering @mergify
Concat arguments when enabling haddock (#10783)
9b140f4 
* Concat arguments when enabling haddock

Arguments were unioned with <> which is left-biased, therefore -haddock
swallowed all other args as indeed described in #10782.

I think the bug was introduced by 80f0a65, which makes the expression
modified in this PR to be non-empty if documentation was enabled.

Fixes #10782

* Add test for #10783 (concatenate arguments when enabling haddock)

---------

Co-authored-by: Matthew Pickering <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit ef29ebb)
@mergify mergify bot mentioned this pull request Mar 28, 2025
Merged
6 tasks
mergify bot added a commit that referenced this pull request Mar 28, 2025
@mergify
Merge pull request #10877 from haskell/mergify/bp/3.14/pr-10783
6cb1e7c 
Backport #10783: Concat arguments when enabling haddock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikolaj Mikolaj Mikolaj approved these changes

@Kleidukos Kleidukos Kleidukos approved these changes

Assignees
No one assigned
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days ready and waiting Mergify is waiting out the cooldown period regression in 3.14 squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Milestone
cabal-install 3.14.2.0
Development

Successfully merging this pull request may close these issues.

Enabling documentation seems to swallow ghc-options passed
7 participants
@jasagredo @Mikolaj @ffaf1 @Kleidukos @mpickering @FinleyMcIlwaine @ulysses4ever