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 Issue 255's Strict Json Transformer #259

Closed
wants to merge 8 commits into from
Closed

Fix Issue 255's Strict Json Transformer #259

wants to merge 8 commits into from

Conversation

JoaquinIglesiasTurina
Copy link

@JoaquinIglesiasTurina JoaquinIglesiasTurina commented Sep 21, 2021

The method for walk :or from spec-tools core was changed.

Previously it was reducing over all the spec's items (different specs within the spec/or).
It was essentialy coercing to a given item and the next given item and so forth and so on.
This caused an strict-json-transformer to behave inconsistently.

@ikitommi 's comment at issue 178, is implemented here.
The method now coerces to all items, and chooses one based on validity or previous behavior.

Issues with the proposed solution

The main issue is that the behavior of the method changes. As it only applies a particular coercion now.

Testing Composed
In that particular test, the value under keys ["keys" ::c2] gets coerced to a keyword.
That is not in thespec.

On the same example ["keys" :c1] will not get coerced to an int now. But that is how coerce works
as ilustrated here.

Parametrization Matters
Minimal examples have been given for this particular issue. Those have been adapted as tests.
However, the spec must be written in a particular way. I find this a major flaw.
This is the clearest example.

@JoaquinIglesiasTurina JoaquinIglesiasTurina changed the title Issue 255 strict json transformer Fixe Issue 255's Strict Json Transformer Sep 21, 2021
@JoaquinIglesiasTurina JoaquinIglesiasTurina changed the title Fixe Issue 255's Strict Json Transformer Fix Issue 255's Strict Json Transformer Sep 21, 2021
@JoaquinIglesiasTurina JoaquinIglesiasTurina closed this by deleting the head repository Apr 21, 2024
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.

1 participant