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

Make empty selector message more helpful #2610

Merged
merged 8 commits into from
Jun 16, 2023
Merged

Make empty selector message more helpful #2610

merged 8 commits into from
Jun 16, 2023

Conversation

ysthakur
Copy link
Contributor

@ysthakur ysthakur commented Jun 14, 2023

Regarding #2601. Currently, if no arguments are given to mill, it says Selector cannot be empty. It's not immediately clear what that's supposed to mean, so I figured it might be more helpful to print the help message instead in that case (some other tools do that). In addition to this, I modified the empty selector message to say Selector cannot be empty. Try `mill resolve _` to see what's available. (the empty selector message still shows up if you do ./mill --repl).

@ysthakur ysthakur changed the title Print help message if no arguments given Make empty selector message more helpful Jun 14, 2023
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

I think showing a typical --help text without request isn't an expected user experience. If there is an issue, we should tell it the user. There is a large gap between some usage hint and an error message with details.

main/client/src/mill/main/client/MillClientMain.java Outdated Show resolved Hide resolved
@lefou
Copy link
Member

lefou commented Jun 15, 2023

While reviewing this PR, I recognized that we left some traces of --repl suport in the code base. I opened #2611 to fix that.

@ysthakur
Copy link
Contributor Author

Okay yeah, I guess an error message would be better than just usage instructions. Does Selector cannot be empty. Try `mill resolve _` to see what's available. look okay? Maybe instead of saying "selector," it could say you need a target? (since it's not immediately clear what a selector is)

@lefou
Copy link
Member

lefou commented Jun 15, 2023

Does Selector cannot be empty. Try `mill resolve _` to see what's available. look okay?

I think so.

Maybe instead of saying "selector," it could say you need a target? (since it's not immediately clear what a selector is)

How about "Target selector must not be empty ..."

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you!

@lefou lefou merged commit 532b8d2 into com-lihaoyi:main Jun 16, 2023
37 checks passed
@lefou lefou added this to the 0.11.1 milestone Jun 16, 2023
@ysthakur ysthakur deleted the empty-selector branch June 16, 2023 22:55
@lefou lefou linked an issue Jun 26, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error message for empty selector
2 participants