You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/12/12 02:07:05 UTC
[GitHub] [avro] KhrystynaPopadyuk opened a new pull request, #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
KhrystynaPopadyuk opened a new pull request, #2009:
URL: https://github.com/apache/avro/pull/2009
## What is the purpose of the change
This PR is my next attempt to brings flexibility and opportunity to override default logic for .NET reflect reader and writer.
Please find more detail in https://issues.apache.org/jira/browse/AVRO-3603
As there is argument against using interfaces (https://github.com/apache/avro/pull/1940) so I made all properties, fields and methods in ClassCache, DotnetClass, DotnetProperty virtual. Also I update all private to protected to be able use and override in child implementation. This should allow similar flexibility as interface.
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## Documentation
- Does this pull request introduce a new feature? NO
- If yes, how is the feature documented? N/A
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] vivere-dally commented on pull request #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
Posted by GitBox <gi...@apache.org>.
vivere-dally commented on PR #2009:
URL: https://github.com/apache/avro/pull/2009#issuecomment-1396715467
Can you please merge and release this PR?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] zcsizmadia commented on pull request #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
Posted by GitBox <gi...@apache.org>.
zcsizmadia commented on PR #2009:
URL: https://github.com/apache/avro/pull/2009#issuecomment-1351525170
My biggest concern is calling the virtual functions from the constructors (see CodeQL complains). I must be addressed before merging.
Personally , I feel interfaces are a better solution here, but I understand the concerns about them either.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KhrystynaPopadyuk commented on pull request #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on PR #2009:
URL: https://github.com/apache/avro/pull/2009#issuecomment-1354061185
Hi @martin-g ,
Current PR does not update serialization/deserialization or other core avro functionality. It is only about classes that parse/map dotnet type to avro schema.
Also, event though this changes can be user to quickly handle errors and add desired functionality, this is not the main purpose.
Users/projects can have custom requirements that are not applicable for everyone and that should not be added to Apache.Avro.
For example, at my project we use camel case naming convention for classes and properties (MyClass, MyProperty) for .NET, and there is requirement to use snake case naming for avro schemas across company (different languages) (my_class, my_property). Currently I use AvroField attribute to override property name. But this mean, each micro service should reefer to Apache.Avro and I should not forget to add that attribute to each property and not forget to update it when schema is changed (what happens often before release). After this PR I will be able to update property - field mapping to add implement custom requirements once.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KhrystynaPopadyuk commented on pull request #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on PR #2009:
URL: https://github.com/apache/avro/pull/2009#issuecomment-1396768811
Hi @vivere-dally ,
This PR and https://github.com/apache/avro/pull/1940 are different approaches to resolve the same task. Only one should be merged.
I prefer https://github.com/apache/avro/pull/1940
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #2009:
URL: https://github.com/apache/avro/pull/2009#issuecomment-1351574932
> virtual functions from the constructors
I think those are not introduced with this PR.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KhrystynaPopadyuk commented on pull request #2009: Avro 3603 dotnet reflect make properties and method virtual in ClassCache, DotnetClass, DotnetProperty
Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk commented on PR #2009:
URL: https://github.com/apache/avro/pull/2009#issuecomment-1354052235
Hi @zcsizmadia ,
I remove "virtual" from methods that are called from constructor. Also, I made fields readonly as CodeQL suggested.
Not sure why this changes are not reflected in scan result.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org