On 07/04/16 15:40, Valentine Sinitsyn wrote: > On 04.07.2016 18:28, charles king wrote: >> 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? > I don't think you should rework all the docstring in this patch series. > However it doesn't make much sense to introduce new future refactoring > spots either. > > So I'd suggest to convert comments to docstrings in this patch, whenever > appropriate. Ack. Rest of the code can be converted later on. > > Ralf, do you agree? Yep.
Ralf > > Valentine >> >> 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(-) >>>>>> >>>>> >>> -- Ralf Ramsauer PGP: 0x8F10049B -- 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.
