Skip to content

Conversation

@auduchinok
Copy link
Member

Fixes #4030.

@auduchinok auduchinok force-pushed the builder-creation-errors-severity branch 3 times, most recently from 5124346 to 2cdd6f5 Compare December 16, 2017 14:27
@auduchinok auduchinok force-pushed the builder-creation-errors-severity branch from 2cdd6f5 to 70318c7 Compare December 16, 2017 14:32
@auduchinok auduchinok changed the title Filter incremental builder creation errors according to compiler args Report builder creation errors according to compiler args Dec 16, 2017
@auduchinok auduchinok force-pushed the builder-creation-errors-severity branch 2 times, most recently from d5326c3 to 4f0a180 Compare December 16, 2017 16:44
@auduchinok
Copy link
Member Author

auduchinok commented Dec 16, 2017

It turned out CompilationErrorLogger didn't change error severities according to error severity options. I fixed it and now I'm waiting for the tests results to check whether anything else needs to be fixed.

@auduchinok auduchinok force-pushed the builder-creation-errors-severity branch from 4f0a180 to d21b248 Compare December 16, 2017 17:00
@TIHan TIHan requested a review from dsyme December 16, 2017 19:55
Copy link
Contributor

@saul saul left a comment

Choose a reason for hiding this comment

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

Can you also add a test case for —warnaserror-:##? This makes a warning a warning when warnings as errors is enabled.

@auduchinok
Copy link
Member Author

@saul Done.

@saul
Copy link
Contributor

saul commented Dec 17, 2017

Cheers! Looks great

@auduchinok auduchinok changed the title Report builder creation errors according to compiler args Report builder creation warnings according to compiler args Dec 17, 2017
Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Thanks for this.

| None -> failwith "declaration list is empty"


[<TestCase(([||]: string[]), ([||]: bool[]))>]
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't urgent, but just to say that I personally never use TestCase attributes in the F# tests, and prefer a loop over an F# list of test case data inputs. Indeed I usually remove any uses of TestCase when I come across them later

While using TestCase attribute has some advantages (e.g. they get reported as separate tests in a test explorer), I believe that just using F# code to program the space of test cases is more uniform and in the long run easier to understand - and encourages people to be really expansive in how they meta-program up a very broad range of systematic test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Programming up the test case matrix with F# data is also more typesafe of course)

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

Great job!

@KevinRansom
Copy link
Contributor

Thanks for this.

@KevinRansom KevinRansom merged commit f1a8978 into dotnet:master Dec 22, 2017
KevinRansom pushed a commit that referenced this pull request Dec 22, 2017
* fix resource name (#4151)

otherwise instead of expected `FSStrings.resources` will use `FSharp.Compiler.Service.netstandard.FSStrings.resources`

* Use ConcurrentDictionary in ImportMap (#4148)

* Fix IndexOutOfRangeException in check for providing completion (#4138)

* Fix IndexOutOfRange in check for providing completion:

* Add test

* Remove repeating arguments processing in IncrementalBuilder creation (#4124)

* Report builder creation warnings according to compiler args (#4125)

* Filter incremental builder creation errors according to compiler args

* Fix CompilationErrorLogger ignores WarsAsError options

* Add test for WarnAsError

* Cleanup

* Add more tests; cover WarnAsError-, no warnings at all

* Refactor tests

* Add test
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
* fix resource name (dotnet#4151)

otherwise instead of expected `FSStrings.resources` will use `FSharp.Compiler.Service.netstandard.FSStrings.resources`

* Use ConcurrentDictionary in ImportMap (dotnet#4148)

* Fix IndexOutOfRangeException in check for providing completion (dotnet#4138)

* Fix IndexOutOfRange in check for providing completion:

* Add test

* Remove repeating arguments processing in IncrementalBuilder creation (dotnet#4124)

* Report builder creation warnings according to compiler args (dotnet#4125)

* Filter incremental builder creation errors according to compiler args

* Fix CompilationErrorLogger ignores WarsAsError options

* Add test for WarnAsError

* Cleanup

* Add more tests; cover WarnAsError-, no warnings at all

* Refactor tests

* Add test
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.

4 participants