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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: comparable function #345

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

Conversation

ariskemper
Copy link
Contributor

No description provided.

@fabian-hiller
Copy link
Owner

Why do you think this is necessary? comparableArray and comparableObject are only used once and the previous comparable functions seams not to complicated to me.

@fabian-hiller fabian-hiller self-assigned this Jan 4, 2024
@fabian-hiller fabian-hiller added the question Further information is requested label Jan 4, 2024
@ariskemper
Copy link
Contributor Author

ariskemper commented Jan 4, 2024

I think it's a bit easier to understand for newcomers on the project and since it is used across the project. That's why those 2 functions are private and exist only in the module. I also didn't like use of any type and change it to be more generic and i used unknown instead any in Record type.

@fabian-hiller
Copy link
Owner

Maybe there is a way to avoid comparable altogether. At least that is what I would expect from a testing framework.

@ariskemper
Copy link
Contributor Author

ariskemper commented Jan 5, 2024

@fabian-hiller it could be solved with JSON.stringify and i removed comparable.

@fabian-hiller
Copy link
Owner

JSON.stringify() is a good idea. Let's keep that in mind when rewriting the tests. I will give you feedback on the email tests today.

@ariskemper
Copy link
Contributor Author

@fabian-hiller i allready rewrote tests and remove comparison function in this PR :).

@fabian-hiller
Copy link
Owner

fabian-hiller commented Jan 6, 2024

One thing to note is that if the properties are in a different order, the JSON.stringify() solution breaks. This was perhaps the reason I implemented comparable.

@ariskemper
Copy link
Contributor Author

ariskemper commented Jan 6, 2024

@fabian-hiller indeed good to note that, do you think is this a feature or issue? :)

@fabian-hiller
Copy link
Owner

The order should not matter. That's why we should probably stick with comparable if there is no other solution.

@ariskemper
Copy link
Contributor Author

ariskemper commented Jan 6, 2024

@fabian-hiller we could use extend matchers and add extra method toCompare? extending-matchers

@fabian-hiller
Copy link
Owner

Can you send some sample code showing how you would implement it?

@ariskemper
Copy link
Contributor Author

ariskemper commented Jan 7, 2024

@fabian-hiller i implemented changes with custom vitest matcher toEqualSchema in this PR. I was not sure regarding naming for toEqualSchema matcher or maybe toCompare?

@fabian-hiller
Copy link
Owner

Thank you! This looks great. I will review it once I made some more progress in other areas of the library.

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.

None yet

2 participants