Skip to content

Conversation

@Happypig375
Copy link
Member

No description provided.

@dsyme
Copy link
Contributor

dsyme commented Jul 9, 2021

Need to fix up the test and get it green, but this is great, thank you.

@KevinRansom
Copy link
Contributor

@dsyme, I'm wondering if this should be an error, because if we should ever choose to implement the functionality declared by these attributes then it would be a breaking change.

@auduchinok
Copy link
Member

@KevinRansom Making it an error where it'd previously compile fine would be a breaking change itself.

|> FSharp
|> typecheck
|> shouldFail
|> withResults [
Copy link
Member

Choose a reason for hiding this comment

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

Just a note - a less verbose withDiagnostics can be used here, for example, like in the ErrorMessages suite:

|> withDiagnostics [
(Error 767, Line 8, Col 16, Line 8, Col 23, "The type Foo contains the member 'MyX' but it is not a virtual or abstract method that is available to override or implement.")
(Error 17, Line 8, Col 18, Line 8, Col 21, "The member 'MyX : unit -> int' does not have the correct type to override any given virtual method")
(Error 783, Line 6, Col 11, Line 6, Col 14, "At least one override did not correctly implement its corresponding abstract member")]

Copy link
Member Author

Choose a reason for hiding this comment

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

@vzarytovskii How can I generate them? For withResults I can just copy from the test output.

Copy link
Member

Choose a reason for hiding this comment

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

@vzarytovskii How can I generate them? For withResults I can just copy from the test output.

I'm afraid, you'll have to write them manually :(
Let me see if I can serialise them into something which you can just copy-paste into withDiagnostics.

@vzarytovskii vzarytovskii merged commit 5af4584 into dotnet:main Jul 12, 2021
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.

5 participants