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

[WIP] Add uniqueItems action #870

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nix6839
Copy link

@nix6839 nix6839 commented Oct 10, 2024

I have some questions.

  1. When I have an array like [5, 6, 5, 5], would it be better to call _addIssue once and add all the duplicate issues related to 5 into the path array at once, or to call _addIssue multiple times to add them individually?"

  2. It seems that uniqueItems in JSON compares objects through deep equality. Should I follow JSON's behavior, or would simple comparison (===) be sufficient?

Resolves #866

@nix6839 nix6839 changed the title Add base of uniqueItems action [WIP] Add base of uniqueItems action Oct 10, 2024
@fabian-hiller
Copy link
Owner

Without much thought I would use dataset.value.length !== new Set(dataset.value).size to check for duplicates and return a single issue. But it is true that this is not an easy decision. We basically have to decide whether we want one issue for the whole array or one issue for each duplicate item. We also have to decide whether to check for shallow equality or deep equality. For example, shallow equality will not work with object because the library parses the input and therefore copies the data into a a new object when the schema is executed. What do you think is best?

@fabian-hiller fabian-hiller self-assigned this Oct 10, 2024
@fabian-hiller fabian-hiller added the enhancement New feature or request label Oct 10, 2024
@nix6839
Copy link
Author

nix6839 commented Oct 11, 2024

We also have to decide whether to check for shallow equality or deep equality. For example, shallow equality will not work with object because the library parses the input and therefore copies the data into a a new object when the schema is executed.

I think it would be best to use deep equality to ensure it operates the same as JSON schema. However, I also believe there will be users who prefer shallow equality. To accommodate those users, how about using deep equality by default but allowing a customizable function in the form of (a, b) => boolean?
I am a bit concerned about the potential performance issues with deep equality. What are your thoughts on this?

We basically have to decide whether we want one issue for the whole array or one issue for each duplicate item.

As for the issue handling, I think we should first agree on and implement the functionality above, and then look into how we can handle the issue format afterwards.

@nix6839 nix6839 changed the title [WIP] Add base of uniqueItems action [WIP] Add uniqueItems action Oct 11, 2024
@fabian-hiller
Copy link
Owner

fabian-hiller commented Oct 11, 2024

How about we provide a uniqueItems action for a small bundle size and better performance and a deepUniqueItems action that uses a nested loop that deeply compares objects, arrays, maps, sets and dates?

@nix6839
Copy link
Author

nix6839 commented Oct 12, 2024

I think that's a good idea. I'll try to implement uniqueItems first tomorrow.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Oct 12, 2024

Would you return an issue for every duplicated item or just one for the entire array? This decision can affect the implementation.

@nix6839
Copy link
Author

nix6839 commented Oct 13, 2024

Since checkItems (which uses a similar naming pattern with -Items instead of -Item) operates by returning an issue for every duplicated item, how about making uniqueItems behave the same way for consistency?

@nix6839
Copy link
Author

nix6839 commented Oct 13, 2024

I've committed it for now, but feel free to share any other opinions. If it's finalized, I'll start working on updating the documentation.

@fabian-hiller
Copy link
Owner

Thank you very much! I will try to give you feedback at the end of next week as I have to focus on other things in the next few days.

@fabian-hiller
Copy link
Owner

I just want to let you know that I am focusing on our v1 release first before reviewing this PR.

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

Successfully merging this pull request may close these issues.

Support uniqueItems in JSON schema?
2 participants