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

an empty target is an empty string #4583

Merged
merged 1 commit into from Jul 5, 2017
Merged

an empty target is an empty string #4583

merged 1 commit into from Jul 5, 2017

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Jul 3, 2017

without this cabal new-run without targets errors with an irrefutable pattern

@mention-bot
Copy link

@fgaz, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @ezyang and @23Skidoo to be potential reviewers.

@23Skidoo
Copy link
Member

23Skidoo commented Jul 3, 2017

/cc @dcoutts

@23Skidoo
Copy link
Member

23Skidoo commented Jul 4, 2017

Looks OK to me, by the way.

@23Skidoo 23Skidoo requested a review from dcoutts July 4, 2017 12:31
@fgaz fgaz mentioned this pull request Jul 5, 2017
Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

So I looked at all use sites of showTargetSelector and it looks like it is only used in error messages. So the change seems to be safe to apply; however, from looking at the TargetSelector definition I don't understand in which cases we can get an empty list here.

@23Skidoo
Copy link
Member

23Skidoo commented Jul 5, 2017

Anyhoozle, I think I'm going to merge this.

@23Skidoo 23Skidoo merged commit d4d4222 into haskell:master Jul 5, 2017
@ezyang
Copy link
Contributor

ezyang commented Jul 5, 2017

I'm generally down on using empty strings as a sigil. Does turning the return type into Maybe cause lots of knock on changes?

@23Skidoo
Copy link
Member

23Skidoo commented Jul 5, 2017

I don't think using Maybe here makes sense, because this function is used during error message formatting, so you'll just end up with a fromMaybe "" at every use site. Maybe it should return something like "<empty>" instead of an empty string.

@fgaz
Copy link
Member Author

fgaz commented Jul 5, 2017

+1, it's called show TargetSelector after all.

The run command is for running a single executable at once. The target '' refers to the package MultipleExes-1.0 which includes the executables foo and bar.
vs
The run command is for running a single executable at once. The target '<empty>' refers to the package MultipleExes-1.0 which includes the executables foo and bar.
The second does look nicer, but the quotes imply <empty> is literally the target.
Of course nobody would interpret it like that. I hope.

(On the other hand with Maybe we could do
The run command is for running a single executable at once. The empty target refers to the package MultipleExes-1.0 which includes the executables foo and bar.)

@ezyang
Copy link
Contributor

ezyang commented Jul 6, 2017

OK, I trust your judgment :)

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.

None yet

5 participants