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 support for homogenous lists #377

Merged
merged 8 commits into from
Jun 1, 2020

Conversation

RiyaJohn
Copy link
Contributor

@RiyaJohn RiyaJohn commented Apr 27, 2020

Issue: #323
Closes #323
Explanation of what this pull request does.
@pelletier
I added logic of GetArray for homogenous lists as getArray func, but need your guidance:

  1. Current changes fails the test cases for Unmarshal support. You wanted a separate function GetArray that users can directly use, but that will mean reusing GetPath() logic again in GetArray with getArray add-on. Is that okay?
  2. Can you also review the getArray logic

More detailed description of the decisions being made and the reasons why (if the patch is non-trivial).

@RiyaJohn RiyaJohn changed the base branch from master to unmarshal-rewrite April 27, 2020 06:49
@RiyaJohn RiyaJohn changed the base branch from unmarshal-rewrite to master April 27, 2020 06:49
@RiyaJohn
Copy link
Contributor Author

@pelletier Please ignore the first 3 commits, not sure why my old PR commits still show up. Files changes only has changes for this pr

@RiyaJohn RiyaJohn marked this pull request as draft April 27, 2020 06:56
@pelletier
Copy link
Owner

Thank you for looking at this! I think the behavior of GetPath should not change (hence why I'd like a new GetArray function. You're right that there will be a duplicate in the way traversing the tree works. I'm ok with it at the moment (eventually we should have a generic way to walk the tree).

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #377 into master will decrease coverage by 0.80%.
The diff coverage is 63.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
- Coverage   95.34%   94.53%   -0.81%     
==========================================
  Files          10       10              
  Lines        2318     2379      +61     
==========================================
+ Hits         2210     2249      +39     
- Misses         68       86      +18     
- Partials       40       44       +4     
Impacted Files Coverage Δ
toml.go 80.59% <63.93%> (-4.92%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16c9a8b...2d866e3. Read the comment docs.

@RiyaJohn RiyaJohn marked this pull request as ready for review May 18, 2020 10:49
@RiyaJohn
Copy link
Contributor Author

RiyaJohn commented May 18, 2020

@pelletier I did not add test cases for homogeneous lists of ints and floats because it picks say, int64 or int32 based on system spec. Hence, test coverage decreases, otherwise all checks have passed. Please review pr and let me know if any changes required

@x-hgg-x
Copy link
Contributor

x-hgg-x commented May 21, 2020

I think toml integers are always parsed to int64 and toml floats are always parsed to float64, so you might safely ignore the cases for int, int32 and float32 (the tree should not contain these types).

Spec:
For integers: 64 bit (signed long) range expected (−9,223,372,036,854,775,808 to 9,223,372,036,854,775,807)

For floats: Floats should be implemented as IEEE 754 binary64 values

@RiyaJohn
Copy link
Contributor Author

RiyaJohn commented May 21, 2020

I think toml integers are always parsed to int64 and toml floats are always parsed to float64, so you might safely ignore the cases for int, int32 and float32 (the tree should not contain these types).

Spec:
For integers: 64 bit (signed long) range expected (−9,223,372,036,854,775,808 to 9,223,372,036,854,775,807)

For floats: Floats should be implemented as IEEE 754 binary64 values

That's great input, thank you. Will make changes

@pelletier pelletier merged commit 2d866e3 into pelletier:master Jun 1, 2020
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.

Better support of lists
3 participants