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

[New] parse: add allowSparse option for collapsing arrays with missing indices #312

Merged
merged 1 commit into from Jan 14, 2021

Conversation

1cheese
Copy link

@1cheese 1cheese commented May 27, 2019

Fixes #181

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This will definitely need some tests.

lib/parse.js Outdated Show resolved Hide resolved
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've rebased this for you.

As for PR requirements, this would also need the readme to be updated.

However, #181 is marked "question" because I'm still not convinced that it's a good idea to create sparse arrays for any reason, and I still believe that all the use cases listed in that issue are better solved by altering the field names.

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #312 (b04febd) into master (b522d2e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #312   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files           8        8           
  Lines        1348     1363   +15     
  Branches      164      166    +2     
=======================================
+ Hits         1346     1361   +15     
  Misses          2        2           
Impacted Files Coverage Δ
lib/parse.js 100.00% <100.00%> (ø)
test/parse.js 99.78% <100.00%> (+<0.01%) ⬆️
test/stringify.js 99.74% <100.00%> (+<0.01%) ⬆️

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 b522d2e...b04febd. Read the comment docs.

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've rebased this, and added stringify tests to match the parse tests that you added (thanks).

I continue to think producing sparse arrays is a bad idea, but since stringify accepts them, it seems useful to enable a round trip.

@ljharb ljharb changed the title Added option for collapsing arrays with missing indices [New] parse: add allowSparse option for collapsing arrays with missing indices Jan 13, 2021
@ljharb ljharb force-pushed the issue_181 branch 2 times, most recently from 36250a4 to b04febd Compare January 14, 2021 04:54
@ljharb ljharb merged commit b04febd into ljharb:master Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to not collapse arrays with missing indices.
2 participants