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

Protobuf schema references support #473

Closed
wants to merge 7 commits into from

Conversation

jjaakola-aiven
Copy link
Contributor

About this change - What it does

Add support for protobuf schema references

References: #195

Why this way

Initial work is in pull request #467
Original #467 pull request description:

These changes add basic support of references/dependencies for protobuf schema type.
This code contains much common code which allows adding support of references/dependencies to other types.
Limitations:
The compatibility check does not support references/dependencies.
Serialization/Deserialization of protobuf does not support references/dependencies.

@jjaakola-aiven jjaakola-aiven force-pushed the protobuf-schema-references-support branch from 9dd1c60 to 0c2b5f0 Compare October 21, 2022 06:08
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 21, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2786858
Status:🚫  Build failed.

View logs

@libretto
Copy link
Contributor

@jjaakola-aiven do You think it is a good idea to develop two different ways of the same functionality? I just have already a workaround for compatibility(compare) functionality in the Instaclustr repository and planning to release it ASAP.

Comment on lines +102 to +108
def compare(
self,
other: "ProtoFileElement",
result: CompareResult,
self_dependency_types: Optional[List[TypeElement]] = None,
other_dependency_types: Optional[List[TypeElement]] = None,
) -> CompareResult:
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a type for dependencies... and my code which I working on already use there exactly this type. please not that Your fixes will interfere with my code logic which I just debugging. You can see it at instaclustr#24

@jjaakola-aiven
Copy link
Contributor Author

@libretto, this PR does not implement the same functionality in different way. Your PR and commit is the basis and my commits are on top of it. I understand this creates a conflict to your implementation, but I have to consider the total quality of the feature and therefore decided to add tests and relevant changes.

@libretto
Copy link
Contributor

libretto commented Oct 21, 2022

@jjaakola-aiven Better add your tests to my latest code where most bugs (including reported by You) are fixed.

@libretto
Copy link
Contributor

libretto commented Oct 27, 2022

@libretto, this PR does not implement the same functionality in different way. Your PR and commit is the basis and my commits are on top of it. I understand this creates a conflict to your implementation, but I have to consider the total quality of the feature and therefore decided to add tests and relevant changes.

@jjaakola-aiven Actually, it did. You did the same work which I did too You can see the code in my PR. It adds more work for me for resolving conflicts and rewriting code which I did in time You did Your changes. You did something in a different way. For example, I have 2 classes one for References and one for Dependencies. I understand You like use List[Reference] instead of References class. It is ok and I will replace it (actually it is not a bad style because karapace has another example of using similar array classes, but ok). By logic, it is better to have Dependency in a separate class because dependencies are resolved references. I understand You want to improve the code style. But why You can not simply tell me to do that by pointing out what I must change? I understand You want to add more tests but why You cannot add them in separate files to avoid conflicts with my code?

@jjaakola-aiven jjaakola-aiven deleted the protobuf-schema-references-support branch September 7, 2023 06:58
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