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

Better error reporting in case of object initializers #1219

Merged
merged 2 commits into from
Dec 16, 2016

Conversation

forki
Copy link
Contributor

@forki forki commented May 25, 2016

fixes #1218

image

@isaacabraham
Copy link
Contributor

Boah. Is that like the quickest F# compiler PR ever? :-)

@forki
Copy link
Contributor Author

forki commented May 25, 2016

only took me so long since I worked on #1220 👅

Now back to that scanner API crap that I'm supposed to fix. I needed short break. because

pile

@forki
Copy link
Contributor Author

forki commented May 25, 2016

@enricosada is there a chance that we print out the err and bsl files if the diff is non-zero? Would help me a lot

@dsyme
Copy link
Contributor

dsyme commented May 25, 2016

We only want to show the new extra message if an "id = expr" is really present in some way.

Could you add a test matrix,e.g.

Person(1) // extra argument, not equality
Person(A=b) // extra argument, id=expr equality
Person(1=b) // extra argument, not-id=expr equality
Person(A=b, C=d) // extra argument, two id=expr equality
Person(A=b
        C=d) // extra argument, two id=expr equality, no comma

and there are probably other cases to add too.

For the wording I suggest:

The object constructor '%s' takes %d argument(s) but is here given %d. The required 
signature is '%s'. If some of the arguments are assigning values to properties, consider 
separating those arguments with a comma (',').

@enricosada
Copy link
Contributor

@forki what test? i think was removed because bsl/diff usually are big and just a single line addition make big diff in the output of test, fsdiff was not too smart with minimize diff.
maybe we can add to test ouput a cmd like kdiff3 path/to/file.bsl path/to/file.err ready to be executed, that's better?

@forki
Copy link
Contributor Author

forki commented May 25, 2016

No the issue is that I don't get to that point locally. I need to see what
was wrong on CI
On May 25, 2016 7:37 PM, "Enrico Sada" notifications@github.com wrote:

@forki https://github.com/forki what test? i think was removed because
bsl/diff usually are big and just a single line addition make big diff in
the output of test, fsdiff was not too smart with minimize diff.
maybe we can add to test ouput a cmd like kdiff3 path/to/file.bsl
path/to/file.err ready to be executed, that's better?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1219 (comment)

@enricosada
Copy link
Contributor

ok got it, because

<test-case name="sigs" executed="True" result="Failure" success="False" time="708.227" asserts="0">
<properties>
<property name="Category" value="typecheck/sigs"/>
<property name="Category" value="Misc02"/>
<property name="Category" value="TypeChecker"/>
<property name="Category" value="TypeCheckerSigs"/>
<property name="DIRECTORY" value="D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs"/>
</properties>
<properties>...</properties>
<failure>
<message>
<![CDATA[
Error running command 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharpqa\testenv\bin\diff.exe' with args 'neg15.err neg15.bsl normalize' in directory 'D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\typecheck\sigs'. ERRORLEVEL 1 ERRORLEVEL 1
]]>

doesnt help when using ci server, right.

So i think the solution can be adding the err/bsl to build artifacts of jenkins/appveyor as additional test results, so can be downloaded (and diff).
i'll try

@forki
Copy link
Contributor Author

forki commented May 25, 2016

Yes that would solve it as well. Thanks
On May 25, 2016 8:01 PM, "Enrico Sada" notifications@github.com wrote:

ok got it, because

...

doesnt help when using ci server, right.

So i think the solution can be adding the err/bsl to build artifacts of
jenkins/appveyor as additional test results, so can be downloaded (and
diff).
i'll try


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1219 (comment)

@forki forki force-pushed the ctors branch 2 times, most recently from 1fb71e4 to a159356 Compare May 30, 2016 08:34
@forki
Copy link
Contributor Author

forki commented May 30, 2016

@dsyme I can't find a way to distinguish the cases

Person(1) // extra argument, not equality
Person(A=b
       C=d) 

So we need to find a way or we adjust the message to include both cases.

@dsyme dsyme changed the title WIP - Better error reporting in case of object initializers [WIP] Better error reporting in case of object initializers Nov 26, 2016
@KevinRansom
Copy link
Member

@forki @dsyme Is there any more to do for this PR? it has been untouched for almost 6 Months.

Thanks

Kevin

@forki
Copy link
Contributor Author

forki commented Dec 14, 2016

Yes as described above I can't find a way to distinguish the two cases and need help

@forki forki force-pushed the ctors branch 7 times, most recently from e226d2a to 6a2c866 Compare December 16, 2016 09:41
@forki forki changed the title [WIP] Better error reporting in case of object initializers Better error reporting in case of object initializers Dec 16, 2016
@forki
Copy link
Contributor Author

forki commented Dec 16, 2016

ok found a way. @KevinRansom this is ready

@forki forki force-pushed the ctors branch 2 times, most recently from d025fad to 38884f3 Compare December 16, 2016 11:20
@KevinRansom KevinRansom merged commit a0014ed into dotnet:master Dec 16, 2016
@forki forki deleted the ctors branch December 17, 2016 09:35
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: Missing , from object initializer
6 participants