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 parse to properly support multipleOf (to avoid validation error) #179

Merged

Conversation

davicorreiajr
Copy link
Contributor

Motivation

#178

@davicorreiajr davicorreiajr changed the title Fix parse of multipleOf (to avoid validation error) Fix parse to properly support multipleOf (to avoid validation error) Mar 28, 2020
Copy link
Collaborator

@AlexanderMann AlexanderMann left a comment

Choose a reason for hiding this comment

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

Very minor nits, otherwise this looks great!


assert singer_stream.peek_invalid_records()
assert singer_stream.count == 0
assert [] == missing_sdc_properties(singer_stream)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can drop this. It's not really adding value to the test overall.

@@ -31,6 +32,8 @@ def test_python_type():
== json_schema.STRING
assert json_schema.python_type('world') \
== json_schema.STRING
assert json_schema.python_type(decimal.Decimal(1)) \
== json_schema.NUMBER
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent

@@ -1,6 +1,7 @@
from copy import deepcopy
import json
import re
import decimal
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't documented anywhere, so I wouldn't expect you to know this one, but we try to alphabetize our imports. The breakdown is:

  • Python
  • 3rd Party
  • Our code

Copy link
Collaborator

@AlexanderMann AlexanderMann left a comment

Choose a reason for hiding this comment

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

Nit which is the result of my commenting on the wrong line originally 😿


assert not singer_stream.peek_invalid_records()
assert singer_stream.count == len(multiple_of_values)
assert [] == missing_sdc_properties(singer_stream)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@davicorreiajr it's this line, we can remove it same as the other, sorry for the confusion and appreciate your patience!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries, fixing now

@AlexanderMann
Copy link
Collaborator

Will merge and hopefully deploy later this week!

@AlexanderMann AlexanderMann merged commit dc1020d into datamill-co:master Mar 30, 2020
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