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

Fixes the issue of stringify encoding comma(used to join array) even when encodeValuesOnly = true #424

Merged
merged 1 commit into from Nov 9, 2021

Conversation

adnan-creator
Copy link

Following changes were made:

  • According to the documentation when encodeValuesOnly = true, then all characters except the values must not be encoded. This includes brackets and joining characters such as comma. It works perfectly for all other characters except when comma is used to join elements of an array. Then even the comma is encoded.
  • One of the tests had a bug where we assumed the result of qs.stringify({ a: ['b', 'c', 'd'] }, { arrayFormat: 'comma' }) to be equal to 'a=b,c,d'. Which is not the expected behavior as for the comma to not be encoded we must either set encode: false or encodeValuesOnly: true. So the right result would be 'a=b%2Cc%2Cd' as the default functionality of the function is to encode the complete string.

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 rebased this on top of my failing tests branch; take a look at the failures. The ??? will need to be updated to an actual expected value.

lib/stringify.js Outdated Show resolved Hide resolved
lib/stringify.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.

Overall this looks great!

Are there any additional test cases we should be adding to make sure this is doing both what the issue participants expect, and what we expect?

@adnan-creator
Copy link
Author

Thanks! for now these tests seem fine.

@ljharb ljharb changed the base branch from comma-fix to master November 9, 2021 19:04
@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #424 (5dbeeb4) into master (e2fd364) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #424   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files           8        8           
  Lines        1388     1393    +5     
  Branches      167      169    +2     
=======================================
+ Hits         1385     1390    +5     
  Misses          3        3           
Impacted Files Coverage Δ
lib/stringify.js 99.29% <100.00%> (+0.03%) ⬆️
test/stringify.js 99.75% <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 e2fd364...5dbeeb4. Read the comment docs.

@ljharb ljharb merged commit 5dbeeb4 into ljharb:master Nov 9, 2021
@adnan-creator adnan-creator deleted the comma-fix branch November 9, 2021 19:39
ljharb added a commit that referenced this pull request Jan 11, 2022
ljharb added a commit that referenced this pull request Jan 11, 2022
ljharb added a commit that referenced this pull request Jan 11, 2022
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.

None yet

2 participants