parse dev mode argument #158
Conversation
RushiT0122
force-pushed
the
TDL-20723-parse-dev-mode-argument
branch
from
bd70722
to
b470f8a
Compare
singer/utils.py
Outdated
| @@ -134,6 +134,7 @@ def parse_args(required_config_keys): | |||
| -d,--discover Run in discover mode | |||
| -p,--properties Properties file: DEPRECATED, please use --catalog instead | |||
| --catalog Catalog file | |||
| -dev, --dev Runs the tap in dev mode | |||
There was a problem hiding this comment.
Is there really any value to having -dev? If we want a short flag, can we pick some other letter?
In some circles, this would read as "I'm passing -d, -e, and -v and POSIX convention says I can chain them together". And this is not something I want us to introduce to this CLI
There was a problem hiding this comment.
Oh yeah, I missed that one. I really to like the convention of having the short flag be only one letter. I'd even be good with -D.
There was a problem hiding this comment.
Updated the short flag -D for dev mode as suggested.
Replace `-dev` to `-D`
singer/utils.py
Outdated
| @@ -163,6 +164,11 @@ def parse_args(required_config_keys): | |||
| action='store_true', | |||
| help='Do schema discovery') | |||
|
|
|||
| parser.add_argument( | |||
| '-dev', '--dev', | |||
There was a problem hiding this comment.
I think it needs to be changed here too
singer/utils.py
Outdated
| @@ -40,7 +40,7 @@ def strptime(dtime): | |||
| ... | |||
| ValueError: time data '2018-01-01T00:00:00' does not match format '%Y-%m-%dT%H:%M:%SZ' | |||
|
|
|||
| Can't parse non-UTC DTs | |||
| Can't parse non-UTC DTsx | |||
There was a problem hiding this comment.
Looks like an errant x committed.
There was a problem hiding this comment.
Deleted errant x.
singer/utils.py
Outdated
| @@ -134,6 +134,7 @@ def parse_args(required_config_keys): | |||
| -d,--discover Run in discover mode | |||
| -p,--properties Properties file: DEPRECATED, please use --catalog instead | |||
| --catalog Catalog file | |||
| -D, --dev Runs the tap in dev mode | |||
There was a problem hiding this comment.
I'd suggest that having a shortcode for this isn't useful since it reduces readability at the savings of 3 characters, maybe if it was --developer-mode or something, but this feels like it would just lead to folks not knowing what the -D means and copy/pasting.
There was a problem hiding this comment.
On that note, maybe not critical right now, but the docstring above the changed lines mentions the SPEC and BEST_PRACTICES documents in the getting started repo, so it might be good to eventually update those.
There was a problem hiding this comment.
Removed the short flag -D.
|
@dmosorast please re-review. |
|
@luandy64 @kethan1122 please re-review. |
Description of change
This implementation adds support to parse dev mode arguments
-Dor--devin cmdline.Manual QA steps
-Dand--devon an existing connection.Risks
Rollback steps