search

igvteam / igv-reports

Python application to generate self-contained pages embedding IGV visualizations, with no dependency on original input files.
MIT License
347 stars 51 forks source link

Add support for sample columns #22

Closed johanneskoester closed 5 years ago

johanneskoester commented 5 years ago

This PR does 3 things:

  1. It adds support for defining sample columns to appear in the report, via e.g. --sample-columns DP PL. Columns will be displayed as samplename:id in the table.
  2. It slightly modifies the CLI to adhere to POSIX standards.
  3. It unifies the formatting in parts of the code.
jrobinso commented 5 years ago

Great work. Thanks. I'm not a native python speaker, and jump between 3 languages. Could you explain why the check "if args.tracks:" is not neccessary (line 29 of report.py), and also why its not neccessary to split the track string (split(',')) before looping through the elements?

jrobinso commented 5 years ago

I get an error if I don't supply sample-columns

Traceback (most recent call last): File "/Users/jrobinso/igv-team Dropbox/James Robinson/projects/igv-reports/igv_reports/report.py", line 169, in main() File "/Users/jrobinso/igv-team Dropbox/James Robinson/projects/igv-reports/igv_reports/report.py", line 165, in main create_report(args) File "/Users/jrobinso/igv-team Dropbox/James Robinson/projects/igv-reports/igv_reports/report.py", line 22, in create_report table_json = table.to_JSON() File "/Users/jrobinso/igv-team Dropbox/James Robinson/projects/igv-reports/igv_reports/varianttable.py", line 55, in to_JSON for h in self.sample_fields: TypeError: 'NoneType' object is not iterable

johanneskoester commented 5 years ago

My fault, I forgot to handle missing args, and assumed that argparse returns an empty list if e.g. --tracks is not specified. But that happens only with nargs="*".

Regarding the question about the splitting: argparse does that automatically for you, if you tell it that an argument has multiple values, via nargs="+". Values are provided space separated then, not comma separated, which also adheres with standard POSIX behavior.

jrobinso commented 5 years ago

OK, thanks for the explanation. This all looks good, but it will probably be next week before I can test it and merge. I'm covered up with some pressing issues on other projects at the moment. The readme will need updated to reflect the new parameter names and space vs comma delimited.

johanneskoester commented 5 years ago

Sure. A suggestion: consider setting up a CI in this repo. Would make it easier to handle pull requests.

jrobinso commented 5 years ago

Yes I will do that eventually, I've done it for my javascript and java projects but not python.

johanneskoester commented 5 years ago

Thanks!