Add full support for the NoSyncTag. - #8312
Conversation
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
Please hold on with the merge, I will do review shortly.
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for doing this. I have just a couple of small items.
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
I think we should turn all init arguments to keyword only, ideally also as an attrs.frozen fields so that validated attributes cannot mutate. We should also default the unset int and bool fields to 0 and False respectively, to mirror how are they handled in the proto; this would let us drop a lot of is None, is not None checks.
Finally, please remove the option for NoSyncTag-type arguments in to_proto and from_proto to be consistent with other SomeTag classes.
…8313) Problem: Building cirq-core, cirq-google, etc. produces `build` directories with a duplicate sources that do not match pylint `ignore-paths`. Solution: Ignore any package build directories first.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8312 +/- ##
========================================
Coverage 99.59% 99.59%
========================================
Files 1125 1127 +2
Lines 103250 103386 +136
========================================
+ Hits 102829 102965 +136
Misses 421 421 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b538fbb to
5ac8815
Compare
5ac8815 to
11155ce
Compare
|
Thanks @pavoljuhas and @mhucka for the review! I updated the PR with your suggestions, PTAL :) |
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Looks good, but before merging, please see the last couple of inline change suggestions.
5 participants
Partial support for the NoSyncTag was added in #7044.
This PR adds full support.