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

Fix `depth: 0`, add `depth: false` #326

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@idler8
Copy link
Contributor

commented Aug 8, 2019

@ljharb Edit: this PR now adds tests for the current useless behavior of depth 0, and then fixes it. Fixes #261.

//当options.depth为空时,按&符进行分割
assert.deepEqual(qs.parse('ids[0]=1&ids[1]=2&ids[2]=3', { depth: 0 }), { 'ids[0]': 1, 'ids[1]': 2, 'ids[2]': 3 })

测试已经通过
我觉得这可以作为一个补充规则

@ljharb

This comment has been minimized.

Copy link
Owner

commented Aug 8, 2019

Google translate says the title is: "Add a rule for depth==0", and the body:

//When options.depth is empty, split by ampersand
assert.deepEqual(qs.parse('ids[0]=1&ids[1]=2&ids[2]=3', { depth: 0 }), { 'ids[0]': 1, 'ids[1]' : 2, 'ids[2]': 3 })
Test passed
I think this can be used as a supplementary rule.

@ljharb ljharb changed the title 增加一个depth==0时的各式规则 Add a rule for depth==0 ( 增加一个depth==0时的各式规则 ) Aug 8, 2019

@ljharb
Copy link
Owner

left a comment

The dist folder is generated, so no changes should ever be made in it. Please make the changes inside lib.

Separately, please provide test cases for this new behavior. Is this a bugfix, or a new feature? I can't tell.

@idler8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

hello
fulfill your requirement.
I first used the "pull request" function, not familiar with it.

@idler8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

travis ci构建失败了?
是不是我的错误?
@ljharb

edit: (ljharb): translation added

Did the travis ci build failed?
Is it my mistake?
@ljharb

@idler8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

#246 #261
是否需要这个补充?
@ljharb

edit (ljharb): translation added

#246 #261
Do you need this supplement?
@ljharb

st.deepEqual(qs.parse('a[0]=b&a[1]=c', { depth: 0 }), { 'a[0]': 'b', 'a[1]': 'c' });
st.deepEqual(qs.parse('a[0][0]=b&a[0][1]=c&a[1]=d&e=2', { depth: 0 }), { 'a[0][0]': 'b', 'a[0][1]': 'c', 'a[1]': 'd', e: '2' });
st.end();
});

This comment has been minimized.

Copy link
@idler8

idler8 Aug 9, 2019

Author Contributor

补充了上面的测试
我觉得这是一个新规则

edit (ljharb): translation added

Added the above test
I think this is a new rule.

@idler8 idler8 changed the title Add a rule for depth==0 ( 增加一个depth==0时的各式规则 ) Add a rule for depth==0 ( 增加一个depth==0时的规则 ) Aug 9, 2019

@ljharb

This comment has been minimized.

Copy link
Owner

commented Aug 9, 2019

I think what we need here is to first add a bunch of tests on master with depth: 0 documenting the existing behavior - and then to rebase this PR on top of it, with your change and the resulting test changes.

@idler8

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

I am sorry.
I will close this PR because my English is too bad.
I can't understand what I can do for you next.

@idler8 idler8 closed this Aug 10, 2019

@ljharb ljharb reopened this Aug 16, 2019

@ljharb ljharb force-pushed the idler8:master branch from ea111f9 to 342ec0c Aug 16, 2019

@ljharb

This comment has been minimized.

Copy link
Owner

commented Aug 16, 2019

I've updated this into two commits; the first that test the current behavior, and the second that make a depth of 0 or false do something intuitive.

I'm split on the semver of this. I could consider it a bugfix, since 0 was surely never intended to act the way it does; i could consider it semver-minor, because it adds depth: false and because it conceptually allows depth to be 0+ instead of 1+; i could consider it semver-major, because someone might be relying on the super weird and useless current behavior of depth: 0.

@ljharb ljharb changed the title Add a rule for depth==0 ( 增加一个depth==0时的规则 ) Fix `depth: 0`, add `depth: false` Aug 16, 2019

@ljharb

This comment has been minimized.

Copy link
Owner

commented Aug 16, 2019

Since I'm adding depth: false, this isn't a patch.

@ljharb

ljharb approved these changes Aug 16, 2019

Copy link
Owner

left a comment

I'm going to go with this one being semver-minor.

If, in fact, someone has found a use case for the current depth: 0 behavior and this breaks them, I may revert that bugfix in the future - but depth: false will remain regardless.

@ljharb ljharb merged commit 649f05f into ljharb:master Aug 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.