refactor: move arg parser to separate file #93
Conversation
|
I like the idea, I think too it is more readable. |
pyreisejl/utility/parser.py
Outdated
| help="The start date for the simulation in format 'YYYY-MM-DD'. 'YYYY-MM-DD HH'. " | ||
| "'YYYY-MM-DD HH:MM', or 'YYYY-MM-DD HH:MM:SS'.", |
There was a problem hiding this comment.
This is somewhat unrelated, but while we're modifying this code let's turn some of these periods into commas where they appear to be part of a list.
pyreisejl/utility/parser.py
Outdated
| parser.add_argument( | ||
| "-e", | ||
| "--end-date", | ||
| help="The end date for the simulation in format 'YYYY-MM-DD'. 'YYYY-MM-DD HH'. " |
There was a problem hiding this comment.
Same here: periods -> commas.
pyreisejl/utility/call.py
Outdated
| default=None, | ||
| help="Scenario ID only if using PowerSimData. ", | ||
| ) | ||
|
|
||
| args = parser.parse_args() |
There was a problem hiding this comment.
The way you kept this line is clever.
pyreisejl/utility/parser.py
Outdated
| "has finished running will be automatically extracted into .pkl files, " | ||
| "and the result.mat files will be deleted. " | ||
| "The extraction process can be memory intensive. " | ||
| "This is optional and defaults to False.", |
There was a problem hiding this comment.
In #88 , I updated this description to defaults to False if the flag is omitted to be a little clearer. We can probably propagate this change here as well.
There was a problem hiding this comment.
Good catch, thanks
pyreisejl/utility/parser.py
Outdated
| import argparse | ||
|
|
||
|
|
||
| def parse_args(): |
There was a problem hiding this comment.
Should this be parse_call_args? I'm assuming there will be a followup PR for extract_data which will need a different argument parsing function.
There was a problem hiding this comment.
Yeah I'll probably have to tweak it a bit, but might end up wrapping these in a class, e.g. from parser import CallParser then doing CallParser.parse_args() so think I will hold off until then if that sounds good.
There was a problem hiding this comment.
Why a class instead of a function? Is there anything stateful we need to keep track of?
There was a problem hiding this comment.
It would be a static method, using the class as more of a namespace than a state holder. Main reason is to have a consistent parse_args() method/function for both use cases.
There was a problem hiding this comment.
That seems like an unnecessary layer over the existing namespace of parser.py. CallParser.parse_args() will already have to be different than ExtractParser.parse_args(), I think parse_call_args() and parse_extract_args() is cleaner.
There was a problem hiding this comment.
Maybe it's just personal preference, it seems more intuitive to me to have {kind of parser}.parse_args()than parse_{kind}_args(). But whichever way we end up going I think we can defer this decision?
There was a problem hiding this comment.
I do see the point about parser.py being the namespace, and it supporting multiple parsers via different functions, so I'm not entirely opposed to that either.
There was a problem hiding this comment.
Six one way, half a dozen the other. If I was reading this fresh, I would assume that there was more to the class than a single method, and be confused when I went looking. But I'm happy to follow whichever option is more popular. @ahurli do you want to cast a tie-breaker vote?
I would rather get the extract arg parsing (in one form or another) into this PR, so that we're not merging this and then immediately starting a parser.py refactor.
There was a problem hiding this comment.
I'm happy to go with the popular option, which I'll tentatively assume is the function based approach that Anna originally suggested.
There was a problem hiding this comment.
I'm not sure how much more complicated we're expecting this to get, so I assume just the function naming convention should be sufficient for now.
But I totally understand waiting for the next PR to implement anything as it's not strictly necessary for this one.
pyreisejl/utility/parser.py
Outdated
| help="The start date as provided to run the simulation. Supported formats are" | ||
| " 'YYYY-MM-DD'. 'YYYY-MM-DD HH'. 'YYYY-MM-DD HH:MM', or 'YYYY-MM-DD HH:MM:SS'.", |
There was a problem hiding this comment.
Let's get these periods to commas too
There was a problem hiding this comment.
Combined with the most recent commit
pyreisejl/utility/parser.py
Outdated
| help="The end date as provided to run the simulation. Supported formats are" | ||
| " 'YYYY-MM-DD'. 'YYYY-MM-DD HH'. 'YYYY-MM-DD HH:MM', or 'YYYY-MM-DD HH:MM:SS'.", |
Purpose
Separate the args definition so it's a bit easier to read call.py. This may also enable some testing but I haven't explicitly done that here.
What it does
Wrap the arg parser into a function and remove the contents of
__init__.pysince I'm pretty sure we don't need them, plus it's out of date anyway.Time to review
3 min - I tested by running
python pyreisejl/utility/call.pywith--helpand1234(scenario id) arguments so think this is safe.