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

stringify: fix arrayFormat comma with empty array/objects #350

Merged
merged 1 commit into from Aug 29, 2020

Conversation

daggerjames
Copy link
Contributor

Fixed #310

continued from PR #317

@ljharb
Copy link
Owner

ljharb commented Jan 15, 2020

Why did you open a second PR? That creates a permanent pollution in the repo refs that can never be removed.

Please never ever make a duplicate PR.

.gitignore Outdated Show resolved Hide resolved
dist/qs.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Owner

ljharb commented May 2, 2020

@daggerjames hey, sorry for the delay here :-)

I've rebased this; the first commit is awesome and good to go; for the second one, 3 of your new tests are failing - and it seems like the tests are correct. Can you take a look?

@ljharb ljharb changed the title #310 stringify: fix arrayFormat comma with empty array/objects May 2, 2020
@daggerjames
Copy link
Contributor Author

fixed all tests. Seems logical error with previous approach.
could you please rebase the branch for me? thanks. I'm still unfamiliar with those rebase things and might lose it like what I did last time

Thanks~

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.

Not stoked on the "use an object as a sentinel key" but it seems straightforward enough, and tests pass.

Thanks, and sorry for the repeated delays :-)

@ljharb ljharb merged commit deada94 into ljharb:master Aug 29, 2020
@jonathansamines
Copy link

Hi @ljharb, sorry to bother you, but were these changes published to npm? I can still reproduce the original behavior from the latest published version.

See this failed test (extracted from this pull request)

@ljharb
Copy link
Owner

ljharb commented Nov 18, 2020

No; this hasn’t been published yet.

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.

arrayformat as comma with empty array or objects
3 participants