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

feat(sort-jsonc): New function: isSortedJsonc() checks if a JSON/JSONC/JSON5 string is sorted #7

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tylerbutler
Copy link
Contributor

@tylerbutler tylerbutler commented Jun 23, 2024

Fixes #6

Adds a new exported function, isSortedJsonc that can be used to check if a JSON/JSONC/JSON5 string is sorted. The implementation short-circuits as soon as an unsorted element is found.

I changed the internal sortDeepWithSymbols function to accept a new parameter, checkOnly, and shortcircuits when that parameter is passed. The typing is configured so that when checkOnly is true, the function returns a boolean. Otherwise, it returns an object like { sorted, alreadySorted: boolean }. The alreadySorted return value is unused, but it seemed possibly useful, so I left it. I can remove it if desired.

Also added an internal isSortedArray function to check if an array is sorted with a short-circuit.

There were no changes to any public APIs except the newly added one, so I added a minor version changeset.

Copy link

changeset-bot bot commented Jun 23, 2024

🦋 Changeset detected

Latest commit: 60f51bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
sort-jsonc Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tylerbutler
Copy link
Contributor Author

I didn't update the readme with docs because I wanted approval that the signature is OK etc. before writing more docs.

Copy link
Owner

@duniul duniul left a comment

Choose a reason for hiding this comment

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

Cool, this would be a useful feature!

As noted in the comment I'm a bit unsure about how much time it will save though, as it's going to have to do extra passes with the compare function until it finds an unsorted key.

What do you think about first checking the entire object (including nested keys) and storing a reference of all unsorted keys, and then only sorting those keys? For isSortedJsonc() you could have an early exist on the first discovered sorted key.

This way you'd at most only do one complete pass over the whole object, while also only applying the sorting where needed. This should make both the current sorting and the isSorted-check more efficient ✅ I could have a look at this in a bit if you don't have time!

packages/sort-jsonc/src/sortJsonc.ts Outdated Show resolved Hide resolved
@tylerbutler
Copy link
Contributor Author

Thanks a lot for looking at this. I appreciate the feedback.

What do you think about first checking the entire object (including nested keys) and storing a reference of all unsorted keys, and then only sorting those keys? For isSortedJsonc() you could have an early exist on the first discovered sorted key.

I like this idea. I was also wondering if it might be faster to strip comments when parsing the JSON just for the isSortedJsonc check. I think you suggested this in an earlier conversation, and it's the approach I took in my very narrowly focused tsconfig sorter/checker, which uses this library to do the actual sort. My code gets to simplify since tsconfig keys are all unique, and I defer to your library to do the actual sort. I didn't do this here because I felt like sharing code would overall be more efficient, but maybe separate code paths wouldn't be too bad if the separate code was relatively simple.

Maybe adding some benchmark tests would be useful, both for evaluating different options but also to share in the readme/track using tests or something. I'll likely need some benchmark setup for my sort-tsconfig project, so I'll try to share that.

This way you'd at most only do one complete pass over the whole object, while also only applying the sorting where needed. This should make both the current sorting and the isSorted-check more efficient ✅ I could have a look at this in a bit if you don't have time!

I won't have much time for a few more weeks, so please feel free to take this and run with it if you want. You can push to this PR branch if you want, or build off of the branch in a new PR, or abandon the code altogether. :) I'll check back in once I have some more time.

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.

feature request: Check if a file is sorted already
2 participants