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

Avoid using to-be-deprecated Data.List.NonEmpty.unzip #8916

Merged
merged 1 commit into from
May 15, 2023
Merged

Avoid using to-be-deprecated Data.List.NonEmpty.unzip #8916

merged 1 commit into from
May 15, 2023

Conversation

mixphix
Copy link
Contributor

@mixphix mixphix commented Apr 22, 2023

For CLC ticket #86


Please include the following checklist in your PR:

Bonus points for added automated tests!

@ulysses4ever
Copy link
Collaborator

Can't we use Data.Functor.unzip? It doesn't have a since annotation, so I'm unsure if it fits into our support window. But that would be much preferable if it's possible imo.

@mixphix
Copy link
Contributor Author

mixphix commented Apr 23, 2023

We'd have to restrict the bounds on base very heavily. I could add a comment about it in the source code, but I hesitate to touch more files than necessary.

@Kleidukos
Copy link
Member

As said in the CLC ticket (haskell/core-libraries-committee#86 (comment)) I'd be more comfortable if we had an automated way to prevent it from coming back, preferably with an HLint or Retrie rule.

@ulysses4ever
Copy link
Collaborator

@mixphix a comment nearby the code you added would be reasonable, I think.

The bounds on `base` prevent us from using `Data.Functor.unzip` here. Its definition has been inlined.
Eventually `Data.List.NonEmpty.unzip` will be monomorphic and this can be reverted with the appropriate `base` bounds.
@ocharles
Copy link
Contributor

The use of Data.List.NonEmpty.unzip is entirely appropriate here, no? Is it just the warning that's problematic?

@mixphix
Copy link
Contributor Author

mixphix commented Apr 24, 2023

The warning is causing the GHC pipeline to fail, see here

@ocharles
Copy link
Contributor

Right, but the code is right so what do you think about turning the warning off, or downgrading it's severity (not sure if the pipeline is looking for -Werror or something else)

@mixphix
Copy link
Contributor Author

mixphix commented Apr 24, 2023

The pipeline has at least -Werror=deprecations. Disabling warnings is a module-wide option that may hide future issues.

@ocharles
Copy link
Contributor

Yea, you'd probably want to downgrade that selectively with CPP.

@mixphix
Copy link
Contributor Author

mixphix commented Apr 24, 2023

You want me to use CPP to avoid a one-line local definition?

@ocharles
Copy link
Contributor

ocharles commented Apr 24, 2023

It's not a project I contribute to, so I don't want you to do anything. I'm just observing what people who use unzip are meant to do, and want to understand what the options are. That said, I do think it's a shame to lose code clarity by now needing to inline the definition

@mixphix mixphix requested a review from ulysses4ever May 3, 2023 17:33
@ulysses4ever
Copy link
Collaborator

Hey @mixphix! Thanks a lot for the contribution! I see you requested my review but i already approved some time ago and that's about all i can do: the PR needs a second reviewer. We could tag some folks who are involved with Cabal but focused on other things and so didn't respond to this PR. E.g. @Kleidukos, @ffaf1, folks, I hope you don't mind me tagging you and perhaps you could find time to review this tiny improvement.

@Kleidukos Kleidukos self-assigned this May 4, 2023
@Kleidukos
Copy link
Member

With ndmitchell/hlint#1500 I am now much more comfortable.

@Kleidukos Kleidukos removed their assignment May 4, 2023
@ulysses4ever
Copy link
Collaborator

@mixphix do you have permissions to add labels, and if so, are you ready to apply the merge-me label to hand it over to the merge bot?

@mixphix
Copy link
Contributor Author

mixphix commented May 4, 2023

I don't have label permissions, but please go ahead!

@Kleidukos Kleidukos added the merge me Tell Mergify Bot to merge label May 4, 2023
@Kleidukos
Copy link
Member

I gotchu

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label May 6, 2023
@Bodigrim
Copy link
Collaborator

What's up with Mergify here?

@Kleidukos
Copy link
Member

Permissions stuff apparently, but let's merge this. :)

@Kleidukos Kleidukos merged commit 3674900 into haskell:master May 15, 2023
kylixafonso pushed a commit to kylixafonso/cabal that referenced this pull request May 21, 2023
@mixphix mixphix deleted the unzip branch June 13, 2023 01:50
@mixphix
Copy link
Contributor Author

mixphix commented Jun 16, 2023

How do I get the GHC MR to include a Cabal version with this patch?

@ulysses4ever
Copy link
Collaborator

@mixphix the usual way with submodules, I think, is to cd into the submodule directory, do git pull, cd back, git add --all and commit...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants