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

Ported FieldMaskUtil from Java to C# #5045

Merged
merged 5 commits into from Oct 8, 2018

Conversation

Projects
None yet
5 participants
@Falco20019
Contributor

Falco20019 commented Aug 14, 2018

Fixes #4325

I ported the functionality from Java to C#. Still a bit unsure if we should keep the classes seperate as Utils or just merging it all into FieldMaskTreePartial (would be getting a huge class...). All other Utils from Java have been ported into the *Partial classes, but those were mainly static classes anyway. The FieldMaskUtil has a complete FieldMaskTree helper class.

Right now, this would allow for easy portability by keeping the Util but also fit with the other classes by creating static helpers in Partial. Any preferences?

@anandolee offered in #4325 to do the PR

@xfxyjwf

This comment has been minimized.

Show comment
Hide comment
@xfxyjwf

xfxyjwf Aug 14, 2018

Contributor

Thanks for putting this together! Note that we are not so happy about the name "FieldMaskUtil" in Java and have a plan to rename it to "FieldMasks". I would recommend against using the name "FieldMaksUtil" in C# just because Java did.

Contributor

xfxyjwf commented Aug 14, 2018

Thanks for putting this together! Note that we are not so happy about the name "FieldMaskUtil" in Java and have a plan to rename it to "FieldMasks". I would recommend against using the name "FieldMaksUtil" in C# just because Java did.

@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Aug 14, 2018

Contributor

@xfxyjwf Thanks for the feedback. Would you prefer to include the FieldMaskTree in FieldMaskPartial (separate partial class), or maybe putting it in the root like JsonFormatter and other helper classes? I think it may be better to only have the stuff directly in FieldMasks and dropping the FieldMaskUtil class completely.

Contributor

Falco20019 commented Aug 14, 2018

@xfxyjwf Thanks for the feedback. Would you prefer to include the FieldMaskTree in FieldMaskPartial (separate partial class), or maybe putting it in the root like JsonFormatter and other helper classes? I think it may be better to only have the stuff directly in FieldMasks and dropping the FieldMaskUtil class completely.

@anandolee

This comment has been minimized.

Show comment
Hide comment
@anandolee

anandolee Aug 24, 2018

Contributor

Can we remove FieldMaskUtil. I noticed FieldMaskPartial has most APIs except IsValid. FieldMaskPartial and FieldMaskUtil are kind of duplicated

Contributor

anandolee commented Aug 24, 2018

Can we remove FieldMaskUtil. I noticed FieldMaskPartial has most APIs except IsValid. FieldMaskPartial and FieldMaskUtil are kind of duplicated

@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Aug 25, 2018

Contributor

I plan to do so, was just waiting on a response to my question. Would you prefer having the FieldMaskTree in a partial or as separate class in root like JsonFormatter?

Contributor

Falco20019 commented Aug 25, 2018

I plan to do so, was just waiting on a response to my question. Would you prefer having the FieldMaskTree in a partial or as separate class in root like JsonFormatter?

@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Sep 4, 2018

Contributor

@anandolee @xfxyjwf Still waiting for feedback on your preference how to separate FieldMaskTree. Either adding it to FieldMaskPartial which would blow it up a bit, or putting it into root like done with JsonFormatter but cluttering up the space there while it's only used by FieldMaskTree and completely internal?

Both are valid ways to proceed with their own up- and downsides. I would prefer the root JsonFormatter-like aproach.

Contributor

Falco20019 commented Sep 4, 2018

@anandolee @xfxyjwf Still waiting for feedback on your preference how to separate FieldMaskTree. Either adding it to FieldMaskPartial which would blow it up a bit, or putting it into root like done with JsonFormatter but cluttering up the space there while it's only used by FieldMaskTree and completely internal?

Both are valid ways to proceed with their own up- and downsides. I would prefer the root JsonFormatter-like aproach.

@anandolee

This comment has been minimized.

Show comment
Hide comment
@anandolee

anandolee Sep 10, 2018

Contributor

I'd prefer to make FieldMaskTree in a separated file instead of blow FieldMaskPartial up. And yes FieldMaskTree should be internal.

Contributor

anandolee commented Sep 10, 2018

I'd prefer to make FieldMaskTree in a separated file instead of blow FieldMaskPartial up. And yes FieldMaskTree should be internal.

Merged FieldMaskUtil into FieldMaskPartial
- Removed FieldMaskUtil
- Moved FieldMaskTree to root
- Updated tests
@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Sep 12, 2018

Contributor

Thanks for the feedback. Just applied the changes.

Contributor

Falco20019 commented Sep 12, 2018

Thanks for the feedback. Just applied the changes.

Improved tests
- Removed internal method FieldMaskTree.GetFieldPaths
- Proof FieldMask.Paths only contains expected values
@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Sep 21, 2018

Contributor

As a short summary:
I prefer Contains and checking the Count over ToString to avoid making assumptions on sorting and have it future proof. Also added assertions to proof specific members are not part of the created path list.

Due to the removal of FieldMaskTree.GetFieldPaths, FieldMaskTreeTest is now having asumptions based on FieldMask and FieldMaskTree.ToFieldMask.

Contributor

Falco20019 commented Sep 21, 2018

As a short summary:
I prefer Contains and checking the Count over ToString to avoid making assumptions on sorting and have it future proof. Also added assertions to proof specific members are not part of the created path list.

Due to the removal of FieldMaskTree.GetFieldPaths, FieldMaskTreeTest is now having asumptions based on FieldMask and FieldMaskTree.ToFieldMask.

@anandolee

This comment has been minimized.

Show comment
Hide comment
@anandolee

anandolee Sep 21, 2018

Contributor

Thanks for the change. It looks good to me, just wait the tests green

Contributor

anandolee commented Sep 21, 2018

Thanks for the change. It looks good to me, just wait the tests green

@anandolee

This comment has been minimized.

Show comment
Hide comment
@anandolee

anandolee Sep 21, 2018

Contributor

Oh for the newly added FieldMaskTreeTest.cs, you need to update Makefile.am to pass distcheck test:
https://github.com/protocolbuffers/protobuf/blob/master/Makefile.am

Contributor

anandolee commented Sep 21, 2018

Oh for the newly added FieldMaskTreeTest.cs, you need to update Makefile.am to pass distcheck test:
https://github.com/protocolbuffers/protobuf/blob/master/Makefile.am

@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Oct 1, 2018

Contributor

@anandolee I hope the distcheck works now :)

Contributor

Falco20019 commented Oct 1, 2018

@anandolee I hope the distcheck works now :)

@anandolee

This comment has been minimized.

Show comment
Hide comment
@anandolee

anandolee Oct 5, 2018

Contributor

csharp/src/Google.Protobuf/FieldMaskTree.cs is also newly added and need to be in Makefile.am

Contributor

anandolee commented Oct 5, 2018

csharp/src/Google.Protobuf/FieldMaskTree.cs is also newly added and need to be in Makefile.am

@Falco20019

This comment has been minimized.

Show comment
Hide comment
@Falco20019

Falco20019 Oct 5, 2018

Contributor

Ok, commited. Is it only giving one error a time? This should be all of the newly added files. If there is anything else to check, just let me know :) (Currently I'm in the same timezone as you which should speed stuff up a bit).

Contributor

Falco20019 commented Oct 5, 2018

Ok, commited. Is it only giving one error a time? This should be all of the newly added files. If there is anything else to check, just let me know :) (Currently I'm in the same timezone as you which should speed stuff up a bit).

@anandolee anandolee merged commit 80e530d into protocolbuffers:master Oct 8, 2018

27 of 28 checks passed

Linux Ruby Kokoro build failed
Details
Bazel Kokoro build successful
Details
Linux 32-bit Kokoro build successful
Details
Linux C# Kokoro build successful
Details
Linux C++ Distcheck Kokoro build successful
Details
Linux Golang Kokoro build successful
Details
Linux Java Compatibility Kokoro build successful
Details
Linux Java JDK 7 Kokoro build successful
Details
Linux Java Oracle 7 Kokoro build successful
Details
Linux JavaScript Kokoro build successful
Details
Linux PHP Kokoro build successful
Details
Linux Python Kokoro build successful
Details
Linux Python C++ Kokoro build successful
Details
Linux Python Compatibility Kokoro build successful
Details
MacOS C++ Kokoro build successful
Details
MacOS C++ Distcheck Kokoro build successful
Details
MacOS JavaScript Kokoro build successful
Details
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
MacOS Obj-C OS X Kokoro build successful
Details
MacOS Obj-C iOS Debug (Allowed Failure) Kokoro build successful
Details
MacOS Obj-C iOS Release (Allowed Failure) Kokoro build successful
Details
MacOS PHP5.6 Kokoro build successful
Details
MacOS PHP7.0 Kokoro build successful
Details
MacOS Python Kokoro build successful
Details
MacOS Python C++ (Allowed Failure) Kokoro build successful
Details
MacOS Ruby 2.1 Kokoro build successful
Details
MacOS Ruby 2.2 (Allowed Failure) Kokoro build successful
Details
cla/google All necessary CLAs are signed

@Falco20019 Falco20019 deleted the Falco20019:add-fieldmaskutil branch Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment