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 "Hint:" to a suggestion message related to coercion failures #1501

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
4 participants
@Armael
Contributor

Armael commented Nov 30, 2017

The goal is to consistently mark suggestions in error messages with "Hint:", as already done with the spellcheck suggestion and the missing rec suggestion.

This change was suggested by @garrigue ; since it's only a one line change i'm not sure a Changes entry is needed..?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 30, 2017

Member

While we are at it, would you maybe give an example of what a double coercion is (in case people don't know) in the error message, "using a double coercion (foo : bar :> baz)."?

Member

gasche commented Nov 30, 2017

While we are at it, would you maybe give an example of what a double coercion is (in case people don't know) in the error message, "using a double coercion (foo : bar :> baz)."?

@Armael

This comment has been minimized.

Show comment
Hide comment
@Armael

Armael Nov 30, 2017

Contributor

Good idea. I added a commit that does that.

Contributor

Armael commented Nov 30, 2017

Good idea. I added a commit that does that.

Show outdated Hide outdated typing/typecore.ml Outdated
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 30, 2017

Member

Good to merge as soon as we stop rebasing for long enough that the CI passes -- the old message may be present in test reference files, so it's not completely obvious that it does.

Member

gasche commented Nov 30, 2017

Good to merge as soon as we stop rebasing for long enough that the CI passes -- the old message may be present in test reference files, so it's not completely obvious that it does.

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Nov 30, 2017

Contributor

I know it wasn't added by this PR, but could we use a better term than "double coercion"? It implies that something is being coerced twice, which isn't the case. Looking through the manual there doesn't seem to be an particular name used for this operator. How about "fully explicit coercion"?

Contributor

lpw25 commented Nov 30, 2017

I know it wasn't added by this PR, but could we use a better term than "double coercion"? It implies that something is being coerced twice, which isn't the case. Looking through the manual there doesn't seem to be an particular name used for this operator. How about "fully explicit coercion"?

@Armael

This comment has been minimized.

Show comment
Hide comment
@Armael

Armael Nov 30, 2017

Contributor

I added an other commit that implements @lpw25 suggestion.

Contributor

Armael commented Nov 30, 2017

I added an other commit that implements @lpw25 suggestion.

@Armael

This comment has been minimized.

Show comment
Hide comment
@Armael

Armael Nov 30, 2017

Contributor

Is the CI stuck or something?

Contributor

Armael commented Nov 30, 2017

Is the CI stuck or something?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Nov 30, 2017

Member

AppVeyor seems stuck, but I'm not too worried about this PR breaking Windows while Travis tests pass. I think this is good to merge. Do you think that you should squash the commits?

Member

gasche commented Nov 30, 2017

AppVeyor seems stuck, but I'm not too worried about this PR breaking Windows while Travis tests pass. I think this is good to merge. Do you think that you should squash the commits?

@Armael

This comment has been minimized.

Show comment
Hide comment
@Armael

Armael Nov 30, 2017

Contributor

I just squashed the commits.

Contributor

Armael commented Nov 30, 2017

I just squashed the commits.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Nov 30, 2017

Contributor

AppVeyor seems stuck, but I'm not too worried about this PR breaking Windows while Travis tests pass.

Here be dragons, and non-updated config/Makefile.m* madness. Although there still seems to be an occasional race condition in the AppVeyor parallel script which I should look into, which I think is more the problem here.

Contributor

dra27 commented Nov 30, 2017

AppVeyor seems stuck, but I'm not too worried about this PR breaking Windows while Travis tests pass.

Here be dragons, and non-updated config/Makefile.m* madness. Although there still seems to be an occasional race condition in the AppVeyor parallel script which I should look into, which I think is more the problem here.

@gasche gasche merged commit 45816d8 into ocaml:trunk Dec 1, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment