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

feat(validation): Add a shared storage between all substates of a ValidateState #105

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bmfs
Copy link
Contributor

@bmfs bmfs commented Jul 9, 2021

This new field is similar to the Misc field from a ValidationState but is shared between all a ValidationState and all its child SubStates.

In the project I'm using this library I have a few custom keywords and I need to be able to identify which are the datapaths that are associated with such keywords.

The easiest way I could think of solving this problem was by having this shared storage at the ValidationState level.
This way, in my custom keyword ValidateKeyword method, I could save the necessary info and have it available as a result of the validation.

@bmfs
Copy link
Contributor Author

bmfs commented Jul 28, 2021

@Arqu do you think this is something that could be merged in?

Copy link
Contributor

@Arqu Arqu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some performance testing just to be on the safe side, has no real impact unless you start hammering the struct itself.

However I think this needs some tests. The struct isn't cleared on a new substate and keys can easily collide depending on usage. Once that's out of the way, I'm happy to merge.

@@ -27,6 +27,9 @@ type ValidationState struct {
Misc map[string]interface{}

Errs *[]KeyError

// ExtraData is a shared storage between all substates of a ValidationState
ExtraData *map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to AdditionalValidationData and its respective comments/function names.

@bmfs
Copy link
Contributor Author

bmfs commented Jul 29, 2021

The struct isn't cleared on a new substate and keys can easily collide depending on usage.

Not clearing the struct was the whole point of adding the new field. That is pretty much the difference to the Misc field.

I'll create a basic test that at least confirms my usecase and make the name changes

@Arqu
Copy link
Contributor

Arqu commented Jul 29, 2021

At the very least lets document that in the comment on the declaration. Not really sold on the use case but like that it would add a way for others to extend the implementation for their needs.

Ie be more explicit that its not cleared and share the key space for its lifetime.

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.

2 participants