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

JsonElement.WriteAsValue does not support scientific notation with decimal separator #39139

Closed
ghord opened this issue Jul 3, 2019 · 25 comments

Comments

@ghord
Copy link

commented Jul 3, 2019

I encountered the issue where following code throws:

var test = JsonDocument.Parse("{ \"test\" : 6.5e-5 }").RootElement;

using(var stream = new MemoryStream())
{
       var writer = new Utf8JsonWriter(stream);
       //throws System.ArgumentException: 'e' is an invalid end of a number. Expected a delimiter.
       test.WriteAsValue(writer);       
}

Tested on .net core 3 preview5 and preview6.

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

if (val == '.')
{
i++;
}
else if (val == 'e' || val == 'E')

Says that . and e are exclusive, which is (in retrospect) obviously wrong.

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Hi
I would like to take this one, is it up for grab?

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@vicrdguez All yours :)

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Added to principal branch, Commit 589cd35
Tell me if something is not correct

@ericstj

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

/cc @tannergooding since this is numerics related

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@vicrdguez I left some comments on the commit. In addition to fixing the functional errors there should be test cases added for

  • Positive decimal positive scientific: 6.022e23
  • Positive decimal negative scientific: 6.022e-23
  • Negative decimal positive scientific: -6.022e23
  • Negative decimal negative scientific: -6.022e-23

I know there's a 1e400 case (positive/positive no-decimal), making sure that we also have something like -1e400 and 1e-400 and -1e-400 seems good... and then integral and decimal with no scientific (1, -1, 1.1, -1.1).

(And a PR is generally a better vessel for comments than individual commit hash links)

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@bartonjs thanks for the corrections, I will add it and create the test cases.
Im sorry for the hash link, is my first contribution.
Thanks for the help!!

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Im having troubles with the projects references since VS can't resolve any. Anywhere I can search about this problem? All the information I found doesn't help.
Also a lot of features throw error:
The feature *feature name* is currently in preview and *unsuported*. To use preview features, use 'preview' language version.

I have the preview sdk and runtime so I can't figure what is wrong

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Are you getting those errors as red squiggles, or compiling is actually failing? (IIRC we have a problem that's being worked on where the VS experience in corefx is broken-looking, but the compile succeeds)

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@bartonjs I have the red squiggles when I load the project, anyways when compiling it fails. First its like the project doesn't find the target framework. I added netcoreapp3.0 by hand in the csproj but then, the references errors appear

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Did you run build.cmd (or build.sh) from the root before loading VS?

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Im doing a fresh instalation of cmake, sdk and runtime. I will then run build.cmd. Let's see if I can make this work.
Thanks!

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

After some testes and re installing everithing, thats what I get when trying to run tests:

[7/9/2019 09:31:33  Error] Testhost process exited with error: It was not possible to find any compatible framework version
The specified framework 'Microsoft.NETCore.App', version '3.0.0' was not found.
  - The following frameworks were found:
      2.1.2 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
      2.1.5 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
      2.1.11 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
      2.1.12 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
      2.2.5 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
      2.2.6 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
      3.0.0-preview6-27804-01 at [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
You can resolve the problem by installing the specified framework and/or SDK.
The .NET Core frameworks can be found at:
  - https://aka.ms/dotnet-download

I have preview six but is like there is another newer installed(?)
When I ran build.cmd I saw this:
dotnet-install: Downloading link: https://dotnetcli.azureedge.net/dotnet/Sdk/3.0.100-preview7-012630/dotnet-sdk-3.0.100-preview7-012630-win-x64.zip

As you can see says preview7

Right after VS2019 notice me about a new update. Maybe the preview7 is being released but not available for downlad yet?

@ericstj

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

You can always run tests from the command-line dotnet msbuild <testProject.csproj> /t:RebuildAndTest after running build.cmd once.

It sounds like the error you're hitting may be related to test refactoring work @ViktorHofer is doing. When running tests from CoreFx in VS the test host shouldn't be running from Program Files, it needs to use the shared framework we've built in the output directory.

If you'd like folks to collaborate more on your changes you can open a draft PR with the suggested change.

@ericstj

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

@vicrdguez can you let us know if you're still looking at this? We really appreciate the help and would like to get this fix in before our lock-down on 7/16. Can you let us know by tomorrow if you plan to move forward with your fix, otherwise we'll pick up this issue ourselves. Thanks!

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@ericstj yes I am! I have all the code an tests, but wasted lots of time with the problem I said yesteday and couldn't run them.
Thanks for the advice running the tests from command prompt, sorry for the delay answering, the time difference is big.
If everithing is correct, tomorrow I will do the commit

@ericstj

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

No worries @vicrdguez, I wasn't sure if you knew about our schedule. Sorry to pressure on the time, normally this sort of issue isn't so urgent but right now we're locking down the product for 3.0. Submit a PR as soon as you have something ready and we'll start giving feedback.

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

No problem @ericstj I totally understand that. Im now finishing and I'm about to commit!
Thanks

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@ericstj I've tried to run the tests in the command prompt since I can't run them in VS,but about 45 tests failed. I don't usually commit changes with this uncertainty but I know you are hurry and I think (and hope) the code is correct.

Let me know if there is something to be fixed.
Thanks!

@ViktorHofer

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

After some testes and re installing everithing, thats what I get when trying to run tests:

How do you run tests inside VS? With the VS Test Explorer? The error that you posted indicates that you are using a wrong dotnet host which would happen if use VS Test Explorer without launching VS from command line, see: #39386. You can either follow the instructions in that link or use F5 (green arrow) to run and debug the tests.

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@ViktorHofer I get that error right clicking in a test method and and then clicking in run tests.
But I see following your instructions I can launch the process.
Whats the difference about the host version between launching vs form command line or not?
Just curious about it

@ViktorHofer

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

I get that error right clicking in a test method and and then clicking in run tests.

That is using VS Test Explorer. As described, this only works when launching VS from the CLI.

Whats the difference about the host version between launching vs form command line or not?

When launching VS from the CLI, we override the PATH property and a few other environment variables temporarily to point to our own built dotnet hive which contains the live built shared framework. If you use VS normally it would use the dotnet host from the dotnet hive at the default location (on Windows usually C:\Program Files\dotnet).

@ericstj ericstj added this to In progress in System.Text.Json - 3.0 Jul 12, 2019

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

I've just commited new changes, waiting for review

@vicrdguez

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

@ViktorHofer I could run tests with F5 (green arrow) but for me, failed to run VS from command line with this error:

.\build.cmd -vs System.Text.Json -arch x64

MSBUILD : error MSB1001: Unknown switch.
Switch: -vs

For switch syntax, type "MSBuild -help"
Build failed.
@ViktorHofer

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Then you are not on latest master. This feature was merged about two days ago.

@ericstj ericstj moved this from In progress to In PR in System.Text.Json - 3.0 Jul 15, 2019

System.Text.Json - 3.0 automation moved this from In PR to Done Jul 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.