Adding an Avro backend #386
Conversation
Conflicts: odo/__init__.py
|
Uh oh, it looks like the |
| @@ -31,6 +31,7 @@ requirements: | |||
|
|
|||
| test: | |||
| requires: | |||
| - avro | |||
There was a problem hiding this comment.
Since this package isn't python 3 compatible, specify it with a conda selector like this:
- avro # [py27]
|
Thanks for the near-instantaneous code review! |
|
No, thank you for the PR! |
| reader = self.reader | ||
| return schema.parse(self.reader.meta['avro.schema']) if reader else None | ||
|
|
||
| uri = property(lambda self: self._uri) |
There was a problem hiding this comment.
any reason not to just make self._uri be self.uri and be a public attribute?
There was a problem hiding this comment.
same with self._codec and self._schema
There was a problem hiding this comment.
uri, schema, and codec feel like properties that should be immutable. If a user changed them, you would need to refresh any open readers and writers. It feels more natural to expect the user to create a new AVRO object rather than reusing an existing one.
| return schema_from_dict(self.reader.schema) if reader else None | ||
|
|
||
| path = property(lambda self: self._path) | ||
| codec = property(lambda self: self._codec) |
There was a problem hiding this comment.
To make these proper data attributes, could you please define a set callable that simply raises AttributeError? See here:
https://docs.python.org/3/howto/descriptor.html#descriptor-protocol
There was a problem hiding this comment.
I'm pretty sure the property function here achieves what you're describing. Would you like me to add a test verifying it, or refactor it into a more standard method format with the @property decorator? I'll just add the test for now.
|
@ahasha OK, first iteration done. |
|
@kwmsmith -- thanks for the code review! I don't have bandwidth to respond to it immediately, but I will make time over the next week or two (hopefully). At any rate, I do plan to get back to this. |
|
Thanks to you for being willing to pick this up again. |
|
@ahasha Thanks for picking this back up -- I'll wait to review / merge this PR until you give me an "all finished". |
|
bump |
|
Hey there. Sorry, but priorities shifted over time and I won't be able to continue work on this PR. (It would make me happy if someone cared enough to finish it and get it merged, though!) |
|
can we conclude this?
i might be able to work on this. |
This PR will fix #370
I'm pretty sure there are edge cases that I haven't thought through, and there are a number of avro types that are not supported (i.e. ['null', 'error_union', 'bytes', 'enum', 'request', 'error', 'fixed']).
However, I figured it is now complete enough to share and get feedback.