PR for retroactive code review #1
Conversation
konradjk
left a comment
konradjk
left a comment
There was a problem hiding this comment.
These are super nit-picky changes. NB: I've only gone through for style and a vague understanding of what the code is generally doing. I haven't dug in fully or tested anything.
obo_parser.py
Outdated
| } | ||
|
|
||
|
|
||
| def convert_obo_to_tsv(input_path, output_path="-", root_id=None, add_category_column=False): |
There was a problem hiding this comment.
I don't like - in a function signature. I'd suggest actually using output_path=sys.stdout instead as the default. And then later checking if output_path is already a file handle. Alternatively, just say output_path="stdout"
tests/test_other_functions.py
Outdated
| import sys | ||
| import unittest | ||
|
|
||
| if sys.version_info > (3, 0): |
There was a problem hiding this comment.
I think should probably be >= in all places
|
|
||
| # remove new-line character and any comments | ||
| line = line.rstrip('\n').split("!")[0] | ||
| if len(line) == 0: |
There was a problem hiding this comment.
I like if not len(line) but doesn't really matter too much.
There was a problem hiding this comment.
I think len(line) == 0 is more readable vs. "not length"
obo_parser.py
Outdated
| continue | ||
|
|
||
| # remove new-line character and any comments | ||
| line = line.rstrip('\n').split("!")[0] |
There was a problem hiding this comment.
Is whitespace generally important on the edges? Generally, line.strip() will catch any other weird whitespace issues.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| p = argparse.ArgumentParser(description="Parse an .obo file and write out a .tsv table") |
There was a problem hiding this comment.
I know it's just a parser, but I still don't really like single letter variable (outside of lambdas)
There was a problem hiding this comment.
what do you usually call it?
This PR covers all code in the initial version of obo_parser