aditya0yadav commented on PR #51: URL: https://github.com/apache/dubbo-python/pull/51#issuecomment-3261184793
Sir please check out all the changes On Thu, 4 Sep, 2025, 19:02 Zaki, ***@***.***> wrote: > ***@***.**** commented on this pull request. > > Please carefully review my comments and either address the issues I raised > or clearly respond in the comments explaining why no changes are made. > ------------------------------ > > In samples/llm/chat_pb2.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322097087>: > > > @@ -1,13 +1,15 @@ > -# -*- coding: utf-8 -*- > # Generated by the protocol buffer compiler. DO NOT EDIT! > > As the hint says, DO NOT EDIT! > ------------------------------ > > In src/dubbo/classes.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322099676>: > > > + def __init__(self, model_type: Optional[type[Any]] = None, **kwargs): > + self.model_type = model_type > + > + @abstractmethod > + def encode(self, data: Any) -> bytes: > + pass > + > + @abstractmethod > + def decode(self, data: bytes) -> Any: > + pass > + > + > +class CodecHelper: > + @staticmethod > + def get_class(): > + return Codec > > Why is an additional CodecHelper wrapper needed? > ------------------------------ > > In src/dubbo/client.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322103015>: > > > request_serializer: Optional[SerializingFunction] = None, > response_deserializer: Optional[DeserializingFunction] = None, > ) -> RpcCallable: > - return self._callable( > - MethodDescriptor( > - method_name=method_name, > - arg_serialization=(request_serializer, None), > - return_serialization=(None, response_deserializer), > - rpc_type=RpcTypes.UNARY.value, > + """ > + Create RPC callable with the specified type. > + """ > + print("2", params_types) > > What is this? > ------------------------------ > > In src/dubbo/client.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322117728>: > > > # Determine serializers > if request_serializer and response_deserializer: > req_ser = request_serializer > res_deser = response_deserializer > else: > - req_ser, res_deser = DubboTransportService.create_serialization_functions( > - codec or "json", # fallback to json > - parameter_types=p_types, > - return_type=r_type, > + req_ser, res_deser = DubboSerializationService.create_serialization_functions( > + codec or "json", > > JSON cannot simply be used as a fallback strategy. Unless you have a > complete decision mechanism, do not set it as a fallback (I already > mentioned this in the previous review...). > ------------------------------ > > In src/dubbo/codec/json_codec/json_transport_base.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322141168>: > > > + self.type_handlers[obj_type] = handler > + > + def register_plugin(self, plugin: TypeHandlerPlugin) -> None: > + """Register a plugin""" > + self.plugins.append(plugin) > + > + def get_handler(self, obj: Any) -> Optional[Callable[..., Any]]: > + """Get handler for object - check dict first, then plugins""" > + obj_type = type(obj) > + if obj_type in self.type_handlers: > + return self.type_handlers[obj_type] > + > + for plugin in self.plugins: > + if plugin.can_serialize_type(obj, obj_type): > + return plugin.serialize_to_dict > + return None > > Same old issue... Please integrate with Dubbo’s extension... > ------------------------------ > > In src/dubbo/codec/json_codec/json_transport_codec.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322151538>: > > > + elif isinstance(obj, date): > + return {"__date__": obj.isoformat()} > + elif isinstance(obj, time): > + return {"__time__": obj.isoformat()} > + elif isinstance(obj, Decimal): > + return {"__decimal__": str(obj)} > + elif isinstance(obj, set): > + return {"__set__": list(obj)} > + elif isinstance(obj, frozenset): > + return {"__frozenset__": list(obj)} > + elif isinstance(obj, UUID): > + return {"__uuid__": str(obj)} > + elif isinstance(obj, Path): > + return {"__path__": str(obj)} > + return {"__fallback__": str(obj), "__type__": type(obj).__name__} > + > > Please refer to the implementation in compression, which is the simplest > extension mechanism, including both interface definition and implementation. > ------------------------------ > > In src/dubbo/proxy/handlers.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322161977>: > > > return self._method_descriptor > > + @staticmethod > + def get_codec(**kwargs) -> tuple: > + return DubboTransportService.create_serialization_functions(**kwargs) > + > + @classmethod > + def _infer_types_from_method(cls, method: Callable) -> tuple: > + try: > + type_hints = get_type_hints(method) > + sig = inspect.signature(method) > + method_name = method.__name__ > + params = list(sig.parameters.values()) > + if params and params[0].name == "self": > > This issue still hasn’t been resolved. > ------------------------------ > > In src/dubbo/codec/protobuf_codec/protobuf_codec_handler.py > <https://github.com/apache/dubbo-python/pull/51#discussion_r2322157729>: > > > +# the License. You may obtain a copy of the License at > +# > +# http://www.apache.org/licenses/LICENSE-2.0 > +# > +# Unless required by applicable law or agreed to in writing, software > +# distributed under the License is distributed on an "AS IS" BASIS, > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > +# See the License for the specific language governing permissions and > +# limitations under the License. > + > +from typing import Any, Type, Protocol, Optional > +from abc import ABC, abstractmethod > +import json > +from dataclasses import dataclass > + > +# Betterproto imports > > This issue still hasn’t been resolved. > > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/dubbo-python/pull/51#pullrequestreview-3185209667>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/BHWNCTKQUGUVUHHMATJOVST3RA5NHAVCNFSM6AAAAACAYF7C2SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTCOBVGIYDSNRWG4> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
