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

AVRO-2577: Fix Bare Excepts #665

Merged
merged 2 commits into from Oct 25, 2019

Conversation

kojiromike
Copy link
Contributor

Jira

  • My PR addresses AVRO-2577
  • My PR references AVRO-2577 in the PR title.
  • My PR does not add any dependencies.

Tests

  • My PR does not add any new tests.

Commits

  • My commits all reference Jira issues in their subject lines.

Documentation

  • My PR does not add new functionality and does not need new documentation.

@@ -408,15 +408,14 @@ def test_parse(self):
for example in EXAMPLES:
try:
schema.parse(example.schema_string)
except (schema.AvroException, schema.SchemaParseException):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fokko I could use some guidance here -- I believe this test is more correct than it was in the previous implementation, but it is now failing on a test case with schema

{"type": "long", "logicalType": "date"}

The implementation allows this to pass on line 921 where it falls back to the primitive type if the logical type doesn't match any earlier condition.

Should we raise an exception whenever a logicalType is specified if it doesn't match the expected primitive?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should raise an exception, because on one side, the spec clearly specifies the required physical type, on the other side, Java validates logical types here, so better to stay consistent across languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @nandorKollar. I've opened AVRO-2580 for this and will address that issue first to avoid mixing contexts in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That change is in #668. Please take a look if you get the chance.

@kojiromike kojiromike merged commit 1f638fb into apache:master Oct 25, 2019
@kojiromike kojiromike deleted the AVRO-2577/fix-bare-except branch October 25, 2019 21:58
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Jan 20, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Jan 20, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Jan 21, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Jan 21, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Jan 22, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba added a commit to kojiromike/avro that referenced this pull request Jan 22, 2020
RyanSkraba pushed a commit to kojiromike/avro that referenced this pull request Jan 27, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba added a commit to kojiromike/avro that referenced this pull request Jan 27, 2020
RyanSkraba pushed a commit to RyanSkraba/avro that referenced this pull request Jan 27, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
RyanSkraba pushed a commit that referenced this pull request Jan 29, 2020
* AVRO-2577: Fix Bare Excepts

* AVRO-2577: Don't Count Failure as Success
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants