Ok,I think the docstrings are very good, but if I add the docstrings in my patch only, the old remained, that is strange, maybe we need a special patch, used for adding all the class docstrings, Maybe could be done in transplanting the python 2.7 to the python 3.0. what you think Valentine? Or just add myself added class only?
best regards from Xuguo Wang 2016-07-04 21:11 GMT+08:00 Valentine Sinitsyn <[email protected]>: > On 04.07.2016 18:06, charles king wrote: >> >> Sorry, Valentine. >> Ralf have mentioned another @staticmethod, I thought it is what you >> remarks, >> so there is no that one. >> Should we reopen another patch to refactor the @staticmethod parse_line? >> Since I thought these remarks' workload is high, could we? > > Yes, it's okay to postpone the refactoring. > But what's wrong with docstrings? They are trivial. > > Valentine > > >> >> best regards >> from Xuguo Wang >> >> 2016-07-04 15:15 GMT+08:00 Valentine Sinitsyn >> <[email protected]>: >>> >>> Hi Xuguo, >>> >>> >>> On 01.07.2016 08:57, Xuguo Wang wrote: >>>> >>>> >>>> *** >>>> All remarks: >>>> 1 Add a whitespace after the comma ', ' >>>> done >>>> 2 # parse /proc/ioports >>>> omit, self-explanatory >>>> 3 # region is larger than two bytes >>>> done >>>> 4 overlay != overlap... >>>> done >>>> 5 Now that I see it: >>>> I can find these three lines of code six times in >>>> this >>>> patch. Isn't >>>> there a chance to consolidate? >>>> It's always the same repeating snippet: >>>> refactored >>>> 6 Amend to sth like 'pad with 0xff, if the last address >>>> is >>>> not 0xffff' >>>> done >>>> 7 This comment doesn't match to the next line. The next >>>> line does not >>>> parse the IOMemRegions, it defines a class... This >>>> comment is - in my >>>> opinion - misleading. >>>> omit, since the code self-explanatory >>>> 8 E.g., I'm not okay with patch 2/3, as it contains a >>>> lot >>>> of redundant >>>> code. >>>> refactor >>>> 9 Comments help, they are useful and necessary for >>>> others >>>> to understand your code. >>>> refactor >>>> >>>> valentine >>>> 1 Any reason for using old-style objects instead of >>>> 'class >>>> IORegion(object)' here? >>>> I think these patch just add the functional of >>>> pio_bitmap generator. >>>> 2 Having regionaclass as the first argument here >>>> strongly >>>> suggests you want >>>> @classmethod, not @staticmethod here. Is there a way >>>> to >>>> refactor your code? >>>> This could be done in the next patch. >>> >>> >>> What do you mean by this quote? I don't see neither changes in your V5, >>> nor >>> counter-arguments here. >>> >>> Valentine >>> >>> >>>> >>>> jan >>>> 1 Please only repost updates when you are done with >>>> them, >>>> and it makes >>>> sense for the reviewer to look at them again. >>>> Otherwise, >>>> they will get >>>> frustrated and spend their time on other things. >>>> done >>>> *** >>>> >>>> Xuguo Wang (3): >>>> tools: Amend the comments. >>>> tools: Refactor the parse_ports. >>>> tools: Template refactor. >>>> >>>> tools/jailhouse-config-create | 407 >>>> +++++++++++++++++++++++++++++++++++++----- >>>> tools/root-cell-config.c.tmpl | 21 +-- >>>> 2 files changed, 369 insertions(+), 59 deletions(-) >>>> >>> > -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
