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

Features/add ignore case enhancement #12

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

NoxPhoenix
Copy link

This adds the option

sortedBy('key', {ignoreCase: true})

in order to get a closer result to alphaNumeric sorting like what returns from a sql db in ORDER BY

@JacobWeyer
Copy link

+1

@johntimothybailey
Copy link
Owner

@NoxPhoenix Good use case example
I'm taking a look right now

@NoxPhoenix
Copy link
Author

Cool, feel free to comment on anything you would like changed. I did my best to follow the style you had in place.

@NoxPhoenix
Copy link
Author

@johntimothybailey How are we looking on this?

@johntimothybailey
Copy link
Owner

@NoxPhoenix thank you for the follow up! Not sure why one of the tests are passing and will comment on the line itself. I'm upgrading the project and dropping support for Node 4, which will help. I'm going to release the upgrades in a few phases.

var useDescendingOrder = typeof options === 'object' ? !!options.descending : false

if (options.ignoreCase) {
return array.map(function (item) {
Copy link
Owner

Choose a reason for hiding this comment

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

@NoxPhoenix While this should cause a failure in the tests I think it passes because it results in being truthy. Nonetheless, this should be array = array.map(function

Copy link
Author

Choose a reason for hiding this comment

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

Ah, yeah. That makes sense. It was completely skipping the assertion. That is a good catch. Pushing the fix now.

@NoxPhoenix
Copy link
Author

That is cool about dropping the node 4 support. I had originally wrote this alot cleaner before I realized it had backwards compatibility that far.

@NoxPhoenix
Copy link
Author

How is this looking now? It would great to get this in before you start your update so that you can update the new code as you refactor the rest!

@NoxPhoenix
Copy link
Author

@johntimothybailey

@saroj990
Copy link

Hi @johntimothybailey, Any update on when this feature is going to be merged?

@NoxPhoenix
Copy link
Author

@johntimothybailey How is this looking?

@pkuczynski
Copy link

@johntimothybailey any chance to get this merged and released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants