Fixing numberformatExceptions for European countries

[NullStress]

No description provided.

[gslowikowski]

I'm wordering, what problem is fixed by this PR.
Anyway, I think it's wrong direction, I would rather forced Locale.US. Scoverage xml files would be portable (I don't know yet, where this could be used) between different machines.

This problem is similar to the problem with encodings (scoverage/scalac-scoverage-plugin#112)

[NullStress]

When you have a JVM with a European local (for many of the european countries) the xml has the values as 12,34 instead of 12.34.

xml.attribute(coverageType.paramName).toDouble() 

will throw a NumberFormatException when it reads a "12,34" String

It seems like it is a similar problem yes, and it might be an idea to force Locale.US

That change would have to be done in scalac-scoverage-plugin though and might affect the other plugins.

Maybe an idea to add an extra method for the write methods that takes Locale as a parameter?

[maiflai]

Sorry, late to this one...

From my side I think that scalac-scoverage-plugin is writing the attribute as a formatted string with the default locale, and hence it may have commas.

I expect the default locale to be consistent between the JVM which is writing the XML, and the JVM which is reading it.

I think the issue is that the Groovy extension toDouble() doesn't seem to respect the default locale, so I agree that the PR takes the appropriate course of action and uses something that does.

I think the PR could be simplified as you don't need to pass the default locale to NumberFormat; it looks as though it will use that automatically.

I'm not sure about the change to the test case as it may have unexpected effects when running tests in parallel.

Finally I don't think we should force the locale of the numeric strings within the XML file; the user's locale is their choice and we should respect that.

I'd like to do a little further digging into the toDouble() issue - is it ok to postpone a release until I've had a detailed look?

Thanks,
Stu

[NullStress]

I think the issue is that the Groovy extension toDouble() doesn't seem to respect the default locale, so I agree that the PR takes the appropriate course of action and uses something that does.

Yes, toDouble does not respect the locale.
http://stackoverflow.com/questions/8285982/parsedouble-in-java-results-to-numberformatexception

I think the PR could be simplified as you don't need to pass the default locale to NumberFormat; it looks as though it will use that automatically.

I didn't notice that it would pick the default locale if nothing was passed, so yes will update that part of the code

I'm not sure about the change to the test case as it may have unexpected effects when running tests in parallel.

I'm not to happy about that either. The issue is that the tests reads in a XML file in the test/resource folder which has numbers formated as "12.34" so those tests will fail for European JVMs

is it ok to postpone a release until I've had a detailed look?

Sure, no problems for me

[gslowikowski]

@maiflai I was wondering, why I have no such problem in Scoverage Maven plugin and compared the sources.
You read and parse scoverage xml files yourself. Why don't you use methods from scalac-scoverage-plugin?

Scoverage cov = ScoverageXmlReader.read(scoverageXmlFile)

statement coverage:

cov.statementCoveragePercent

branch coverage:

cov.branchCoveragePercent

line coverage written to cobertura.xml is actually statement coverage (see https://github.com/scoverage/scalac-scoverage-plugin/blob/master/scalac-scoverage-plugin/src/main/scala/scoverage/report/CoberturaXmlWriter.scala#L77)
so line coverage:

cov.statementCoverage

[maiflai]

I think doing this naively will bring in a dependency on Scala 2.11, and force that dependency on every other plugin in the Gradle build script.

I originally wanted to move this functionality up into the common library space as a command-line application; then execute it in a new JVM with the Scala dependencies isolated.

[maiflai]

I agree that it might be better if the xml files didn't contain locale-specific formatting - perhaps we could change the format string upstream to match the default Double parsing format? I think this is slightly different to picking a region such as Locale.US.

[sksamuel]

We can change the format upstream. Or just lock it down to a specific
format.

On 11 November 2015 at 22:54, maiflai [email protected] wrote:

I agree that it might be better if the xml files didn't contain
locale-specific formatting - perhaps we could change the format string
upstream to match the default Double parsing format?

—
Reply to this email directly or view it on GitHub
#47 (comment)
.

[gslowikowski]

Hi

I finally had time to analyze this problem.

As @maiflai wrote above:

I expect the default locale to be consistent between the JVM which is writing the XML, and the JVM which is reading it.

I agree with this, so whats the point to pass the locale to check task. This is always the current locale. There is no way (for now, I want to provide PR, when I will have more time) to control locale when writing scoverage xml files. The only problem you have is with respecting current locale when reading numbers from xml file.

Additionally, there should be at least two tests with different locales set up, one using dots (Locale.US) and one using commas. Using Locale.setDefault in test setup wasn't wrong. This would test, if plugin works properly with different locales (for different users). Concurrent test execution should be turned off because, as @maiflai wrote, it could cause problems.

Again, sorry for so late comment (after a release).

[maiflai]

Thanks for the comments, they are much appreciated.

I think I favour an upstream fix here; there is no reason for the scalac scoverage plugin to write locale-specific files.

Oddly enough I think this is actually a bug I introduced: maiflai/scalac-scoverage-plugin@9daa437

If the upstream fix is accepted then we can revert to the previous behaviour which is much simpler.

I've also remembered why I chose to parse the XML file; it leaves the way open for users to write their own custom assertions inside the gradle build file. I think doing it in other ways requires a more convoluted approach.