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

Fix slowness #169

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Fix slowness #169

merged 2 commits into from
Oct 19, 2022

Conversation

sobrinho
Copy link
Contributor

We don't need to specify either allowing and rejecting.

Fix #127

We don't need to specify either allowing and rejecting.

Fix igorkasyanchuk#127
@sobrinho sobrinho mentioned this pull request Oct 10, 2022
@gr8bit
Copy link
Collaborator

gr8bit commented Oct 12, 2022

@sobrinho if I understand correctly, we still have a breaking behavior with this PR as @joshmcarthur pointed out in #127, right?

@igorkasyanchuk
Copy link
Owner

@sobrinho this looks like a good solution. I think it worth to merge.

But can you please check if we need this method content_type_keys

image

@sobrinho
Copy link
Contributor Author

Yep, it is a breaking change in terms of not testing the other side of the validator.

@gr8bit
Copy link
Collaborator

gr8bit commented Oct 13, 2022

Yep, it is a breaking change in terms of not testing the other side of the validator.

Ok, but I also think the advantages outweigh that. 👍

@igorkasyanchuk
Copy link
Owner

@gr8bit from your side do you approve this PR?

@gr8bit
Copy link
Collaborator

gr8bit commented Oct 19, 2022

@gr8bit from your side do you approve this PR?

Yes. 👍

@igorkasyanchuk igorkasyanchuk merged commit 7fce7c5 into igorkasyanchuk:master Oct 19, 2022
@sobrinho sobrinho deleted the fix-slowness branch October 19, 2022 12:14
@mvz
Copy link
Contributor

mvz commented Oct 19, 2022

Why exactly is specifying both allowing and rejecting at the same time not allowed anymore? It looks like the rest of the code should handle that just fine.

@sobrinho
Copy link
Contributor Author

I don't recall why I added that but restoring it works just fine.

Added a new PR: #173

@mvz
Copy link
Contributor

mvz commented Oct 20, 2022

Thanks @sobrinho, I actually added a PR for that as well (#172) 🙂

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.

Specs slowed down alot
4 participants