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

Revert "add marked output to cabal list-bin" #8668

Closed
wants to merge 1 commit into from
Closed

Revert "add marked output to cabal list-bin" #8668

wants to merge 1 commit into from

Conversation

andreasabel
Copy link
Member

@andreasabel andreasabel commented Jan 13, 2023 •
edited
Loading

Reverts

Said PR didn't solve #8400 properly and introduced regression #8664. As 3.10 cut is imminent, I propose a swift action here, giving us peace to solve #8400 without time pressure.

Fixes #8664.

Update: I have prepared another PR, #8670, that fixes #8400. If we manage to merge that one quickly, we do not need the present PR.

@andreasabel andreasabel added cabal-install: cmd/list-bin regression on master Regression that is unreleased and needs to be fixed before release labels Jan 13, 2023
Mikolaj
Mikolaj approved these changes Jan 13, 2023
View reviewed changes
Copy link
Member

@Mikolaj Mikolaj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Please kindly emergency-merge after getting 2 reviews, without the waiting period.

Edit: if needed.

@Mikolaj Mikolaj requested a review from ulysses4ever January 13, 2023 08:35
@Mikolaj
Copy link
Member

Mikolaj commented Jan 13, 2023

@Cmdv: I tried to add you to the reviewers field, but I can't because you don't have the proper repo permission. I've sent an invite with these.

This was referenced Jan 13, 2023
Merged
Closed
Cmdv
Cmdv approved these changes Jan 13, 2023
View reviewed changes
Copy link
Collaborator

@Cmdv Cmdv left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@Cmdv
Copy link
Collaborator

Cmdv commented Jan 13, 2023

@Mikolaj In the original PR there was mention this area lacking tests. Any recommendations as to what/where these should look like?

@Mikolaj
Copy link
Member

Mikolaj commented Jan 13, 2023

@Mikolaj In the original PR there was mention this area lacking tests. Any recommendations as to what/where these should look like?

I think @andreasabel has some ideas (and questions) in other tickets.

@Cmdv
Copy link
Collaborator

Cmdv commented Jan 13, 2023

cool I'll have a look I spotted you mentioned there were "some" tests but they were false positives/not catching this case :)

@Mikolaj
Copy link
Member

Mikolaj commented Jan 13, 2023

Yes. We collectively messed up. :)

@Cmdv
Copy link
Collaborator

Cmdv commented Jan 13, 2023

haha we can all point fingers at each other then no one is to blame!! :)

@Mikolaj
Copy link
Member

Mikolaj commented Jan 13, 2023

it wasn't me!!!

@Mikolaj
Copy link
Member

Mikolaj commented Jan 16, 2023

If I'm not mistaken, this one is not needed any more thanks to merging #8670. @andreasabel: amazing search&rescue just in time for 3.10. Closing.

@Mikolaj Mikolaj closed this Jan 16, 2023
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

@Cmdv Cmdv Cmdv approved these changes

@ulysses4ever ulysses4ever ulysses4ever approved these changes

Assignees
No one assigned
Labels
cabal-install: cmd/list-bin regression on master Regression that is unreleased and needs to be fixed before release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cabal list-bin is broken cabal list-bin does not produce marked output
4 participants
@andreasabel @Mikolaj @Cmdv @ulysses4ever