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

Marshmallow Based Event Schema Validation #40

Merged
merged 8 commits into from
Aug 3, 2017

Conversation

piotrekno1
Copy link
Contributor

@piotrekno1 piotrekno1 commented May 8, 2017

Coversation of the discussion started in: #21

I know that for now this looks super ugly but wanted to get your feedback on:

  • Placement of the validation code. Validation is controlled by Repository class.
  • How do you like the Schema registration API? It mimics what is done for mutators.
  • Do you like the fact that only event data is validated (static fields of an event are skipped to avoid repetetive work).

Things that needs to be changed if we agree on the points above:

  • Decide where to place schema definitions in examples (schema.py ?).
  • Add tests
  • Relax the setup.py import
  • Strict mode to enforce validation (?)

@lukaszb
Copy link
Owner

lukaszb commented May 10, 2017

I really like it, plain and simple.

  • Placement of the validation code. Validation is controlled by Repository class.

👍 This way we'd keep Storage.store raw (so it's good for example in testing). People should use Repository for real usage anyway.

  • How do you like the Schema registration API? It mimics what is done for mutators.

Looks cool. I wonder if we can combine two things, though - event definition and mutator. This, however, can be done in a next step.

  • Do you like the fact that only event data is validated (static fields of an event are skipped to avoid repetetive work).

Sure, we can extend that in future but I'm not sure if that would be needed at all. Data is all we should care for now.

@piotrekno1
Copy link
Contributor Author

Please see the proposal.

Copy link
Owner

@lukaszb lukaszb left a comment

Choose a reason for hiding this comment

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

I like it! I have only one request

cq/aggregates.py Outdated
result = schema.load(event_data)
if result.errors:
msg = "Error validatng %s.%s event. Details: %s." % (cls.get_name(), event_name, result.errors)
raise SchemaValidationError(msg)
Copy link
Owner

Choose a reason for hiding this comment

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

please propagate errors in a raised exception instance

@piotrekno1
Copy link
Contributor Author

piotrekno1 commented Aug 2, 2017

@lukaszb Not sure I understood. Does 1d50e99 address your feedback? If so 100% agree.

@lukaszb
Copy link
Owner

lukaszb commented Aug 3, 2017

@piotrekno1 exactly, thanks :)

@lukaszb lukaszb merged commit 5a287e3 into lukaszb:devel Aug 3, 2017
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