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

Improve error message for empty --allow-newer= (fix #7740) #8140

Merged

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented May 14, 2022

This is a WIP, it's not beautiful: (see the first comment below)

❯ $CABAL build --allow-newer=                                                                                              1 (0.018s) < 04:31:50
Error: cabal: invalid argument to option `--allow-newer': "<parsecToReadE>"
(line 1, column 1):
unexpected empty argument list for --allow-newer=. Use --allow-newer without
the equals sign to permit all packages to use newer versions.
expecting "*"

Should "parsecToReadE" be removed? It can be but this may make it harder to debug anything that goes via parsecToReadE.

expecting "*" is annoying but I'm not sure how to remove it either.


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@ulysses4ever ulysses4ever force-pushed the issue-7740-empty-allow-newer-parse branch from 04aaf81 to 838c360 Compare May 15, 2022 03:04
@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented May 15, 2022

Okay, I think it's ready for review now. I found a way to extend the current approach for converting ParsecParser to ReadE that shouldn't break anyone's workflow, and at the same clean up the parsing error message:

❯ $CABAL build --allow-newer=                                                                                                    1 (0.029s) < 04:55:15
Error: cabal: invalid argument to option `--allow-newer': empty argument list
is not allowed. Note: use --allow-newer without the equals sign to permit all
packages to use newer versions.

I didn't notice that error message were tested anyhow. Have I missed it and should add a test? At the moment I'm not sure where to add it.

@ulysses4ever
Copy link
Collaborator Author

Would appreciate if @phadej could take a look, as the parts of code I touched are mostly due to him.

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.

A tiny changelog file would be needed nevertheless. LGTM otherwise. Congrats.

@ulysses4ever ulysses4ever force-pushed the issue-7740-empty-allow-newer-parse branch from 838c360 to 299893c Compare May 16, 2022 23:59
@ulysses4ever
Copy link
Collaborator Author

@Mikolaj done! Thanks for the review!

@ulysses4ever ulysses4ever force-pushed the issue-7740-empty-allow-newer-parse branch 2 times, most recently from 4252c7e to a761d85 Compare May 17, 2022 00:25
@Mikolaj
Copy link
Member

Mikolaj commented May 17, 2022

CI got canceled. Let me try to restart.

@Mikolaj
Copy link
Member

Mikolaj commented May 17, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented May 17, 2022

rebase

✅ Branch has been successfully rebased

@andreabedini andreabedini force-pushed the issue-7740-empty-allow-newer-parse branch from a761d85 to b8c67bb Compare May 17, 2022 08:30
@Mikolaj
Copy link
Member

Mikolaj commented May 17, 2022

No time to lose --- let's merge it (via merge_me label) before somebody says it doesn't make sense.

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

Kleidukos commented May 17, 2022

Jetzt geht's los!

@mergify mergify bot merged commit 8248c3d into haskell:master May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants