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

Add a hint suggesting to use ( * ) when (*) is used #1534

Merged
merged 2 commits into from Dec 15, 2017

Conversation

@Armael
Copy link
Member

@Armael Armael commented Dec 15, 2017

A common rookie mistake is to write (*), trying to get the prefix equivalent of the multiplication function -- the compiler rightfully warns that this is in fact the beginning of a comment, but does not explain how to actually get the prefix version.

This patch simply extends the existing warning in order to suggest using ( * ) instead.

@@ -304,7 +304,8 @@ let () = parse_options false defaults_w;;
let () = parse_options true defaults_warn_error;;

let message = function
| Comment_start -> "this is the start of a comment."
| Comment_start -> "this is the start of a comment.\n\
Hint: Did you mean `( * )'?"

This comment has been minimized.

@Octachron

Octachron Dec 15, 2017
Member

Would it not be a bit clearer to precise that ( * ) is an operator: Did you mean the operator `( * )'? ?

This comment has been minimized.

@Armael

Armael Dec 15, 2017
Author Member

Good point!

@Armael Armael force-pushed the Armael:hint_prefix_mult branch from 4c0c8f2 to b552215 Dec 15, 2017
@gasche
Copy link
Member

@gasche gasche commented Dec 15, 2017

Why not be fully explicit, "the infix operator ( * ) for multiplication"?

@Armael Armael force-pushed the Armael:hint_prefix_mult branch from b552215 to 01b3d6c Dec 15, 2017
@Armael
Copy link
Member Author

@Armael Armael commented Dec 15, 2017

Indeed. Patch updated.

@Octachron
Copy link
Member

@Octachron Octachron commented Dec 15, 2017

Some weak counter-arguments

  • infix: ( * )cannot be used in infix position
  • for multiplication: some misguided people use * for vector product, others may not spontaneously associate convolution ( which is the natural * with distributions ) with *

But I agree that the infix operator ( * ) for multiplication is fine overall

@gasche
Copy link
Member

@gasche gasche commented Dec 15, 2017

I apologize for the bike-shedding, but I think that "(* is the start of a comment" would be a better first line (than "this"). Then for the hint, one possibility (but I'm not sure it is an improvement) would be to say "Hint: If you meant to write the infix operator ( * ) for multiplication, you should add spaces within the parentheses."

@Octachron
Copy link
Member

@Octachron Octachron commented Dec 15, 2017

Maybe This "(* is the start of a comment to avoid starting a sentence with a symbol?

Also, going beyond one line of hint in

Hint: If you meant to write the infix operator ( * ) for multiplication, you should add spaces within the parentheses.

feels a bit verbose.

@gasche
Copy link
Member

@gasche gasche commented Dec 15, 2017

Yep, "This (*" is nice. I think that it is good to be a bit demonstrative.

For the second sentence, my idea was to say explicitly "you need to add spaces", but if there is no space (sic) for this maybe just showing the symbol without a space above and the symbol with spaces below is enough.

@Octachron
Copy link
Member

@Octachron Octachron commented Dec 15, 2017

Then maybe we should put the focus on the actual human mistake:
Did you forget spaces when writing the infix operator `( * )`
(with optionally for multiplication)

@gasche
Copy link
Member

@gasche gasche commented Dec 15, 2017

Excellent!

@Armael Armael force-pushed the Armael:hint_prefix_mult branch from 01b3d6c to 37b413e Dec 15, 2017
@Armael
Copy link
Member Author

@Armael Armael commented Dec 15, 2017

Thanks for the discussion. I just amended the patch.

@gasche
Copy link
Member

@gasche gasche commented Dec 15, 2017

(Why are you using double-backtick in the first line, and backtick-singlequote in the second line?)

@Armael Armael force-pushed the Armael:hint_prefix_mult branch from 37b413e to 1636b6a Dec 15, 2017
@Armael
Copy link
Member Author

@Armael Armael commented Dec 15, 2017

(Oops, fixed)

@gasche gasche merged commit a7fcf04 into ocaml:trunk Dec 15, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damiendoligez
Copy link
Member

@damiendoligez damiendoligez commented Feb 22, 2018

Just a remark for next time: backtick and single quote are not a pair of matched quotation marks, so you shouln't use them in this way. The problem is rather well explained in this page: http://www.cl.cam.ac.uk/~mgk25/ucs/quotes.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.