Project

General

Profile

Task #23090

Pull Request review

Added by Yuyi Guo about 1 month ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Target version:
-
Start date:
08/08/2019
Due date:
% Done:

100%

Estimated time:
Duration:

Description

Hi Vito,

I create a yuyi_PR branch for the code review. Would you please review it?

Thanks,
yuyi

History

#1 Updated by Vito Di Benedetto about 1 month ago

  • % Done changed from 0 to 100
  • Status changed from New to Resolved

Hi Yuyi, below are my comments about this PR.


python/project_py/xml2cfg.py
  • 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

python/project_py/xml2ini.py

  • if code in lines 117-126 will be removed, we also need to remove code in line 13 and 111

python/project_py/sam_dataset.py

  • line 102: typo, Same -> Some

template/generic/template_campaign.ini

  • line 22: vo_role=Analysis has been removed, is this field optional?

bin/poms_cli.py

  • line 92: replacing --jobs with --njobs could give better idea of what this option is used for.
    maybe replacing:
    'number of jobs to submit. This is to overwrite the number of jobs on stage configure. will only used for submitting jobs by stage'
    with:
    '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.


Also available in: Atom PDF