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

Allow strings to be split into arrays if desired #233

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paul42
Copy link

@paul42 paul42 commented Oct 24, 2017

more to scratch my own itch, but I was wondering if anyone else wanted to have the following functionality.
100% coverage, added entries to readme (love how that parses, btw) and tests for parse. tried to follow what I think was the coding conventions in the repo.

huge fan of the library, this is just a rough cut, can I get some feedback on it when you have a moment?

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 is a different take on #219; it may be helpful to read that.

lib/parse.js Outdated
options.depth = typeof options.depth === 'number' ? options.depth : defaults.depth;
options.arrayLimit = typeof options.arrayLimit === 'number' ? options.arrayLimit : defaults.arrayLimit;
options.parseArrays = options.parseArrays !== false;
options.splitArrays = options.splitArrays !== false;
Copy link
Owner

Choose a reason for hiding this comment

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

in this case, parseArrays isn't the pattern to follow; this should ensure it's === true instead. As is, this would be a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

Hey thanks for taking the time to respond, I'm going to read through #219 and come back - I'm not dead set on any particular pattern, just happens to be my first pass, so please let me know if you see any potential issues or areas that desire more testing.

Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

ah, so a function to split the value into arrays would remove the need for settings sprawl?

Copy link
Owner

Choose a reason for hiding this comment

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

yes, exactly

@paul42
Copy link
Author

paul42 commented Oct 26, 2017

is this more along the lines of what you were thinking?

@ljharb
Copy link
Owner

ljharb commented Oct 26, 2017

Looks much closer! We should add the same function to stringification too.

@paul42
Copy link
Author

paul42 commented Oct 26, 2017

gotcha, let me take a crack at it

@paul42
Copy link
Author

paul42 commented Oct 26, 2017

@ljharb looking at this and #219 I think a solution could possibly be allowing an arrayFormatter parameter which would be a function, and if it's passed in it would supersede other options (like arrayPrefixes of brackets vs indices) and if it's not passed in or null, then the library operates as before - does that sound reasonable? I imagine it'll be a more advanced feature for those who want to fine tune their operation

@paul42
Copy link
Author

paul42 commented Nov 5, 2017

I'm also wondering if - to allow the function to act on all the items in the array, we should put the call to the function passed in up at https://github.com/ljharb/qs/blob/master/lib/stringify.js#L80 instead of inside the for loop for each key?

@ljharb
Copy link
Owner

ljharb commented Nov 5, 2017

The function is called recursively, and it's important that it continue to do that - I think sticking with one key at a time is probably better.

@paul42
Copy link
Author

paul42 commented Nov 5, 2017

I'm having a bit of block seeing how to get from where it is to where I need to be on it - let me poke at it more, sorry for all the hand-holding that's required I'm still not super awesome at JS, so it's taking me a bit longer. I think the mental block I'm trying to over come is understanding how these lines work:
https://github.com/ljharb/qs/blob/master/lib/stringify.js#L91-L92
I'm not 100% on the syntax, so I don't follow what the second generateArrayPrefix does if nothing's passed in?

I think maybe I'll just go back and look up how to run the debugger and step through it a ton of times until I get a solid understanding of it.

@ljharb
Copy link
Owner

ljharb commented Nov 5, 2017

In that case it just passes the function itself, since functions are first-class values in JS.

@paul42
Copy link
Author

paul42 commented Nov 6, 2017

so - in the case of object being an array, that value, 'generateArrayPrefix' gets passed parameters, does work, and returns - but in the case of not-an-array, the function is just passed in that order as a place holder so further recursive calls have it on hand to call? I think I'm grasping it, so I really can use the callsite you mention in 219. 💡 let me poke at it a bit more, thanks for the clarification.

@dan-lee
Copy link

dan-lee commented Nov 26, 2017

I'd really love to have this feature! Did you make some progress @paul42?

@paul42
Copy link
Author

paul42 commented Nov 26, 2017

hi @dan-lee sorry about the delay, but I'm having trouble wrapping my head around how to collapse the values into a single unit after it's reading the next key. if you want a really rough version you can try my branch, but it's not on npm and I haven't had a ton of time to work on this. I'll probably poke at it a bit more and then ask @ljharb for help again

@michaelblash
Copy link

michaelblash commented Dec 2, 2017

I'm really longing for customisable array stringification as well.

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