[GitHub] [beam] InigoSJ commented on pull request #12074: [BEAM-10313] Add without_defaults to built-in Combiners in Python

[GitBox <gi...@apache.org>]

InigoSJ commented on pull request #12074:
URL: https://github.com/apache/beam/pull/12074#issuecomment-648932828


   @pabloem 
   
   Some questions that I would need help with:
   
   1 - Is the name `CombinerWithoutDefaults` good enough for the parent class? Am I adding it in the right place?
   2 - All my tests passed, but just to double check, am I missing any `*args, **kwargs` anywhere? 
   3 - I needed to add `timestamp = 0` in many tests, should I do it as a global variable? maybe just use 0 without referencing `timestamp`?
   4 - Assert at *795*: I used a bigger window (180) so that there's 2 outputs. Should I keep it as the previous assert that doesn't have windows (i.e., with one output element)?
   5 - Checking the file, I don't see any pydoc that's needed, but maybe you think I should add something
   
   Thanks a lot for your help!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org