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

toml: update the alexcrichton and BurntSushi repos to their successors toml-rs, and toml-test, record new exceptions #21152

Merged

Conversation

spytheman
Copy link
Member

@spytheman spytheman commented Mar 31, 2024

Fix #20933 .

@spytheman spytheman requested a review from larpon March 31, 2024 10:24
@spytheman spytheman changed the title toml: update https://github.com/alexcrichton/toml-rs.git to https://github.com/toml-lang/toml-test.git, record new exceptions toml: update the alexcrichton and BurntSushi repos to their successors toml-rs, and toml-test, record new exceptions Mar 31, 2024
@spytheman
Copy link
Member Author

@larpon, sorry for the noise. I realized too late, that the successor repo for alexcrichton, can be used directly, since it also contains the commit 499e8c4, which we need.

I think the PR should be stable enough for review now.

Copy link
Contributor

@larpon larpon left a comment

Choose a reason for hiding this comment

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

Hi @spytheman thanks for taking care of this. The changes look fine to me - the CI job definitely looks a lot nicer now. Did you have a specific reason to not rename burntsushi/alexcrihton, if those tests are now the official tests?

I only have one issue locally:

All separate tests (alexcrichton_toml_rs_test.v, burntsushi_toml_test.v iarna_toml_spec_test.v and large_toml_file_test.v) pass on my system (EndeavourOS Arch/rolling) when run separately, but running:

  VTEST_TOML_DO_YAML_CONVERSION=1 VTEST_TOML_DO_LARGE_FILES=1 ./v test vlib/toml

... locally gives me this:
image

Note the e vs. E I'm not sure if that is because I'm on newer versions of the software stack or what 🤔

I remember we had to do some shoehorning / tweaking of some test data/results because of the YAML conversions I'm not sure if this might be one of those 🤔

@larpon
Copy link
Contributor

larpon commented Apr 1, 2024

image

From https://github.com/vlang/v/pull/21152/files#diff-f9e436e6d2d57cf88c41f996f2c4be49302cc0559cd810f254cc7fcb7536bae1R343-R391
EDIT
From

ast.Number {
if value.text.contains('inf') {
mut json_text := value.text.replace('inf', '1.7976931348623157e+308') // Inconsistency ???
if skip_value_map {
return '${json_text}'
}
return '{ "type": "float", "value": "${json_text}" }'
}
if value.text.contains('nan') {
mut json_text := 'null'
if skip_value_map {
return '${json_text}'
}
return '{ "type": "float", "value": "${json_text}" }'
}
if !value.text.starts_with('0x')
&& (value.text.contains('.') || value.text.to_lower().contains('e')) {
mut val := '${value.f64()}'.replace('.e+', '.0e') // json notation
if !val.contains('.') && val != '0' { // json notation
val += '.0'
}
if skip_value_map {
return '${val}'
}
return '{ "type": "float", "value": "${val}" }'
}
v := value.i64()
// TODO: workaround https://github.com/vlang/v/issues/9507
if v == i64(-9223372036854775807 - 1) {
if skip_value_map {
return '-9223372036854775808'
}
return '{ "type": "integer", "value": "-9223372036854775808" }'
}
if skip_value_map {
return '${v}'
}
return '{ "type": "integer", "value": "${v}" }'
}

So it might be a tool update that might have fixed/changed the scientific notation.

Also seems we've closed the i64(-9223372036854775807 - 1) special case: #9507
But it's probably better to look into that in a separate clean up

@spytheman
Copy link
Member Author

spytheman commented Apr 1, 2024

I could not get the problem to show up locally @larpon , and it seems it is also passing on the CI. I suspect that the used jq version may be the culprit. My local copy is jq-1.6.

Edit: on the CI, it is also jq-1.6.

@spytheman
Copy link
Member Author

Did you have a specific reason to not rename burntsushi/alexcrihton, if those tests are now the official tests?

Just minimization of the commit diff. I did not want to have both renames and code changes in the same PR.

@spytheman spytheman merged commit ef48fb6 into vlang:master Apr 1, 2024
42 checks passed
@spytheman spytheman deleted the fix_toml_test_repo_location_issue_20933 branch April 2, 2024 15:34
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.

outdated official toml test
2 participants