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

203 validator custom message error causes test matcher error #206

Conversation

Mth0158
Copy link
Collaborator

@Mth0158 Mth0158 commented Oct 27, 2023

@igorkasyanchuk here is a refacto proposition to make tests clearer.

In my opinion, switching to spec makes it really easier to read the tests, plus a big part of the community is using Rspec as their testing framework, so writing tests like specs makes even more sense.

I have just done the refacto for the size_validator_matcher as a POC, let me know if that's okay with you. Then, I was thinking of proceeding the following way:

Let me know how I can be helpful

@igorkasyanchuk
Copy link
Owner

@Mth0158 I'm good with this refactoring, please continue 🚀

@Mth0158
Copy link
Collaborator Author

Mth0158 commented Nov 3, 2023

@igorkasyanchuk I think I'll:

And maybe we can merge it if that's okay with you?
I will refactor the tests on the validators in another PR, since this PR is already adding a lot (as of now, it makes the test suite up to 360 from 232).

@igorkasyanchuk
Copy link
Owner

@Mth0158 I'm fine with it. If you want to merge - do it.

@Mth0158
Copy link
Collaborator Author

Mth0158 commented Nov 8, 2023

@igorkasyanchuk & @gr8bit I finished this PR.
I first added a lot of tests to make sure the big refacto in the last commit does not cause regressions.
The last commit is all about refactoring to simplify and DRY the code, it also fixes the issue #201 and add new inside features that can be reused somewhere else (the Validatable module, the Symobolizable module especially).
Let me know what you think of this, maybe it's better to only focus on the last commit since the other ones are all about cleaning and updating the test suite.

@Mth0158
Copy link
Collaborator Author

Mth0158 commented Nov 8, 2023

I still think we could go further in refactoring the code, but I'll stop here for this PR, it's big enough (make too big actually) 😅

@igorkasyanchuk igorkasyanchuk merged commit bd26c11 into igorkasyanchuk:master Nov 10, 2023
26 checks passed
@igorkasyanchuk
Copy link
Owner

@Mth0158 merged, thanks for the tremendous work. Do you think it worth to release a new version, or we can just wait.

@Mth0158 Mth0158 deleted the 203-validator-custom-message-error-causes-test-matcher-error branch November 10, 2023 09:26
@Mth0158
Copy link
Collaborator Author

Mth0158 commented Nov 10, 2023

@igorkasyanchuk thanks for the merge!
Maybe we can release it as a minor version so it definitely fixes the bug mentioned in #203 & #201 that some users had. I do not know exactly how you plan releases so let me know what you think is best in this use case.
It's not a critical release anyway, more something that will be useful to work with going on

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.

Validator custom error message causes test matcher error
2 participants