Skip to content

Conversation

@forki
Copy link
Contributor

@forki forki commented May 30, 2016

fixes #1112

image

@forki forki force-pushed the prop branch 2 times, most recently from f8e0677 to 9bf4a80 Compare May 31, 2016 06:48
@forki forki changed the title WIP: Improve error reporting: property setter in constructor expression Improve error reporting: property setter in constructor expression May 31, 2016
@forki
Copy link
Contributor Author

forki commented May 31, 2016

I think this one is ready for review.

@KevinRansom
Copy link
Contributor

Looks, good, probably the message needs tweaking though:

The object constructor 'MyClass' has no argument or settable property named 'Property'. The required
signature is new : unit -> MyClass.
Consider:
MyProperty
MyProperty2
ABigProperty

@cartermp
Copy link
Contributor

cartermp commented Jun 1, 2016

Suggested fix: getting the type signature in single quotes. This would be consistent with other PRs.

The object constructor 'MyClass' has no argument or settable property named 'Property'. The required signature is 'new : unit -> MyClass'.
Consider:
MyProperty
MyProperty2
ABigProperty

@isaacabraham
Copy link
Contributor

I thought we were dropping ' for types?

Kevin- do we really need "object constructor" or can "constructor" suffice?

@forki
Copy link
Contributor Author

forki commented Jun 2, 2016

@KevinRansom the "maybe you want part" is from the little helper that we brought in for "elm like" suggestions. We should change it to something that works everywhere. I agree the text should start with "Consider" since that is what we did for other messages as well.

@cartermp
Copy link
Contributor

cartermp commented Jun 2, 2016

@isaacabraham

I thought we were dropping ' for types?

I'm wasn't aware of this. In that case, would we then want to update other PRs to not longer use ' for types, such as #1219 and #1229? I'm fine with any approach so long as it's consistent.

@forki
Copy link
Contributor Author

forki commented Jun 2, 2016

I think we didn't really come to a conclusion about wrapping types in ',
yet. But since generic types ''a' look weird there is room to improve
that.
On Jun 2, 2016 6:29 PM, "Phillip Carter" [email protected] wrote:

@isaacabraham https://github.com/isaacabraham

I thought we were dropping ' for types?

I'm wasn't aware of this. In that case, would we then want to update other
PRs to not longer use ' for types, such as #1219
#1219 and #1229
#1229? I'm fine with any
approach so long as it's consistent.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1228 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNJV3kaSH2yAGSJ4NRJ3bZhpH-cazks5qHwTbgaJpZM4Ip5Hk
.

@isaacabraham
Copy link
Contributor

Sorry - wasn't my intention to come across so strong there :) Agree 100% consistency - thought ppl had agreed upon no quotes but I guess I wasn't correct there :)

@isaacabraham
Copy link
Contributor

One thing I wish we had done before we started this is define the style of error messages we wanted, like slightly refined from what we have now or the whole shebang like Elm.

@cartermp
Copy link
Contributor

cartermp commented Jun 2, 2016

@isaacabraham

Sorry - wasn't my intention to come across so strong there :)

No worries 😄

One thing I wish we had done before we started this is define the style of error messages we wanted, like slightly refined from what we have now or the whole shebang like Elm.
Show all checks

Let's kick off that discussion in #1103! We can outline what we think a good overall style can look like, and how to approach things like generic type signatures.

@KevinRansom
Copy link
Contributor

@isaacabraham I'm okay with us iterating towards the best style. Already we understand a lot more about these messages than we did at the start. The bug fixes are easy to implement, try out and discuss. I think we are now in a much better state than we were and the more bugs we look at the better our understanding.

Right now we quote replaceable types in messages. We could change that, it is however, a painful experience, given the various different types of tests we have.

@forki
Copy link
Contributor Author

forki commented Jun 30, 2016

rebased

@dsyme dsyme merged commit 23ef783 into dotnet:master Jul 21, 2016
@dsyme
Copy link
Contributor

dsyme commented Jul 21, 2016

Merged on behalf of @KevinRansom who is heads-down in tuples, and @otawfik-ms who is heads down in Roslyn Workspaces :)

Thanks for the great contribution @forki, and for @isaacabraham too for kicking off this error message initiative

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.

Improve error reporting: property setter in constructor expression

6 participants