search

BD2KGenomics / ga4gh-integration-deprecated

Tracking for ga4gh-integration projects
1 stars 2 forks source link

Add protocol to common (or schemas) #46

Closed david4096 closed 7 years ago

david4096 commented 7 years ago

The protocol functions for coercing to and from JSON would be nice to package with the schemas so that projects don't need to get protobuf themselves if they want to do serialization. @dcolligan

dcolligan commented 7 years ago

Yes, I was looking at that as well. That would probably make sense. The reason I didn't do that initially was because there's a lot of other stuff in protocol (for instance SearchResponseBuilder) but that could be refactored out into another module. The schemas repo would probably be the best place for it.

dcolligan commented 7 years ago

On the other hand, if we are shoving more stuff into the ga4gh.schemas.* namespace, it may be good to keep the schema pb2 files under a ga4gh directory...

How do you think the namespace should be structured if we add protocol (and perhaps more things in the future?) into ga4gh.schemas? Keep in mind we already have a non-pb file, pb.py under ga4gh.schemas.

david4096 commented 7 years ago

Very good questions. The place I looked first was pb.py to see if the json_format functions were there. Perhaps we can move the protocol buffers specific stuff there. The stuff that is relevant only to the server would stay.

ga4gh.schemas.variant_service_pb2 is a desirable state, nesting another level deeper is pretty confusing. I think it's OK if we add our own protocol to the same namespace. Refactoring protocol.py into pb.py or vice versa makes sense to me (while leaving the server specific stuff). Added bonus, remove the _pb2 from the module names?

from ga4gh.schemas.variants_pb2 import variants
from ga4gh.schemas import pb
v = variants.Variant(id="abc")
pb.toJson(v)
dcolligan commented 7 years ago

Fixed a while ago

david4096 commented 7 years ago

Thanks @dcolligan https://github.com/ga4gh/ga4gh-schemas/pull/801