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

allowDots option for stringify. Tests also updated. #151

Merged
merged 4 commits into from
Feb 4, 2016
Merged

allowDots option for stringify. Tests also updated. #151

merged 4 commits into from
Feb 4, 2016

Conversation

snow01
Copy link
Contributor

@snow01 snow01 commented Feb 3, 2016

Added allowDots option for stringify. I have added a new test case for the same.

@tdzienniak
Copy link
Contributor

I think that some array stringification test would be good to make.

@snow01
Copy link
Contributor Author

snow01 commented Feb 3, 2016

@tdzienniak -- Following three tests are enough and expected ?

  • { a: { b: ['c', 'd'] } } ===> a.b[0]=c&a.b[1]=d
  • { a: [{ b: 'c' }] } ===> a[0].b=c
  • { a: [{ b: { c: [1] } }] } ===> a[0].b.c[0]=1

@@ -65,9 +65,9 @@ internals.stringify = function (object, prefix, generateArrayPrefix, strictNullH
}

if (Array.isArray(obj)) {
values = values.concat(internals.stringify(obj[key], generateArrayPrefix(prefix, key), generateArrayPrefix, strictNullHandling, skipNulls, encode, filter));
values = values.concat(internals.stringify(obj[key], generateArrayPrefix(prefix, key), generateArrayPrefix, strictNullHandling, skipNulls, encode, filter, sort, allowDots));
Copy link
Owner

Choose a reason for hiding this comment

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

it looks like you caught a minor bug/omission with the missing "sort" param here and below - i'm not sure if that was intentionally omitted or not originally. could you also add tests that cover the presence/absence of this parameter (here and below)?

Alternatively, if you could prepare a separate PR that adds the sort param and the tests for it, i'd prefer to merge that first and keep this PR focused on "allowDots" :-)

@snow01
Copy link
Contributor Author

snow01 commented Feb 3, 2016

@ljharb - Yep, I saw 'sort' param missing as a bug due to omission. Existing tests are not capturing that bug. At depth 3 or more, without that sort does not work.

input --
{ a: 'a', z: { zj: {zjb: 'zjb', zja: 'zja'}, zi: {zib: 'zib', zia: 'zia'} }, b: 'b' }

Without 'sort' param fix --

a=a&b=b&z[zi][zib]=zib&z[zi][zia]=zia&z[zj][zjb]=zjb&z[zj][zja]=zja

With sort param fix --

a=a&b=b&z[zi][zia]=zia&z[zi][zib]=zib&z[zj][zja]=zja&z[zj][zjb]=zjb

Committing the test for this case in same PR, hope okay.

@ljharb
Copy link
Owner

ljharb commented Feb 3, 2016

Thanks, that's helpful. I'd still prefer a separate PR but I won't make it a sticking point :-)

@snow01
Copy link
Contributor Author

snow01 commented Feb 4, 2016

Lint is giving error for 3 reasons --

  • more number of parameters ===> we may need to pass parameters as opts.
  • more number of statements ===> can refactor some.

/github/qs/lib/stringify.js
23:23 error This function has too many parameters (9). Maximum allowed is 8 max-params
77:18 error Function 'anonymous' has a complexity of 19 complexity
77:18 error This function has too many statements (33). Maximum allowed is 32 max-statements

From PR perspective how do you suggest we do this so lint passes ?

@ljharb
Copy link
Owner

ljharb commented Feb 4, 2016

I'm fine just bumping all those limits in .eslintrc. I'll worry about tightening them down later.

@snow01
Copy link
Contributor Author

snow01 commented Feb 4, 2016

Rather than bumping up values, should I change those settings to warning ?

@ljharb
Copy link
Owner

ljharb commented Feb 4, 2016

Nah, warnings are pretty useless imo since they're easy to ignore.

@snow01
Copy link
Contributor Author

snow01 commented Feb 4, 2016

Cool. Done.

Please pass it soon, if possible, so I can move from dependency on my branch to NPM package.

Thanks !

ljharb added a commit that referenced this pull request Feb 4, 2016
[New] allowDots option for `stringify`
[Fix] "sort" option should work at a depth of 3 or more
@ljharb ljharb merged commit b191b37 into ljharb:master Feb 4, 2016
@ljharb ljharb added this to the 6.1.0 milestone Feb 4, 2016
@ljharb
Copy link
Owner

ljharb commented Feb 9, 2016

@snow01 Released as v6.1.0

@snow01
Copy link
Contributor Author

snow01 commented Feb 9, 2016

Thanks Jordan !

@TheSharpieOne
Copy link
Contributor

Needs documentation added to the readme. Had to search the repo to find this to figure out this feature already existed and then view the changes to see how to use it.

@ljharb
Copy link
Owner

ljharb commented Jan 17, 2017

@TheSharpieOne a PR or an issue for updating the docs would be appreciated

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.

4 participants