-
Notifications
You must be signed in to change notification settings - Fork 73
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
Conversation
9dd1c60
to
0c2b5f0
Compare
@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. |
def compare( | ||
self, | ||
other: "ProtoFileElement", | ||
result: CompareResult, | ||
self_dependency_types: Optional[List[TypeElement]] = None, | ||
other_dependency_types: Optional[List[TypeElement]] = None, | ||
) -> CompareResult: |
There was a problem hiding this comment.
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
@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 Better add your tests to my latest code where most bugs (including reported by You) are fixed. |
@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? |
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: