Pull Request review
I create a yuyi_PR branch for the code review. Would you please review it?
#1 Updated by Vito Di Benedetto over 1 year ago
- % Done changed from 0 to 100
- Status changed from New to Resolved
Hi Yuyi, below are my comments about this PR.
- line 252: maxfilesperjob doesn't need to be present in the XML, if it is missed, it is assumed to be 1. Generation stages do not need to set it as well.
- line 261: The logger.error argument could be split on multiple logger.error lines, so it is easier to read on the terminal
It also looks like all the leading spaces for second and third message lines are printed in the final message on the terminal..
- several places: if we detect a 'convert_or_not = False', could we just abort the conversion with an exit statement? This way we can avoid to check that flag later in the code
- line 291: typo, templet -> template
- line 477: it looks like the 'try' section has two indentation level, i.e. 8 column instead of 4
- line 486: cs_split_type and dataset_or_split_data are from the .ini file, while self.cfg is the cfg object.
- line 605: if we abort the conversion in case 'convert_or_not' is false, we can take out this check here
- if code in lines 117-126 will be removed, we also need to remove code in line 13 and 111
- line 102: typo, Same -> Some
- line 22: vo_role=Analysis has been removed, is this field optional?
- line 92: replacing --jobs with --njobs could give better idea of what this option is used for.
'number of jobs to submit. This is to overwrite the number of jobs on stage configure. will only used for submitting jobs by stage'
'number of jobs to submit. This overwrite the number of jobs in the stage configuration. Can be used only to submitting jobs by stage'
Also here I think leading spaces are printed on terminal.