TIMOB-9309: BlackBerry: Convert argparse to optparse part2 #74
Conversation
Reviewers: DavidL, JP ChangeLog: - Convert to optparse for blackberry.py blackberryndk.py scripts Test Cases: - Create sample project using project.py. - Make sure project successfully created. - Make sure blackberry and blackberryndk scripts correctly run the unit testa with -t option. - Make sure created project can be successfully built and run.
| parser.add_option('--id', help='Blackberry project id', dest='id') | ||
| parser.add_option('--dir', help='Blackberry project directory', dest='dir') | ||
| parser.add_option('--ndk', help='Blackberry NDK path', dest='ndk') | ||
| parser.add_option('-t', '--test', help='run unit tests', metavar='ndk_location', action='store_const', const=True, dest='test') |
There was a problem hiding this comment.
I know this isn't part of your patch, but is there a reason there's metavar='ndk_location' for --test?
There was a problem hiding this comment.
The reason was to format the usage saying that could be provided ndk_location with -t option. Don't know do we need that really.
There was a problem hiding this comment.
But -t doesn't have a value and we provide the ndk location with --ndk
There was a problem hiding this comment.
Its for unit testing only. you can provide -t or -t with ndk path.
There was a problem hiding this comment.
Yes, but I don't think that's what metavar is for. I could be wrong. Either way it doesn't seem to affect the functionality but I wanted to bring it up.
There was a problem hiding this comment.
Why wouldn't the ndk path just be specified using --ndk even when -t is specified? Seems like that would be more consistent. Having [--ndk ndk] [-t [ndk_location]] in the usage is confusing
There was a problem hiding this comment.
the confusion comes from having two params that take ndk.
-t shouldn't take any param and --ndk value should be used for the ndk path in all cases (normal use and unit tests).
I see no reason for specifying the ndk with the -t option
|
Approved |
| @@ -165,30 +164,30 @@ def __runUnitTests(): | |||
| print '\nFinished Running Unit Tests' | |||
| UnitTest.printDetails() | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
For the future please refrain from making changes to the empty lines format, it will never end if people start adding and removing empty lines back and forth to suit their personal preference.
Don't need to revert this.
|
Maybe not for this patch, but project.py is actually the most important file that needs to be changed to use optparse, and devicemanagement.py also will need to be modified to use optparse |
|
Reviewed |
|
The project.py & devicemanagement.py will be updated as a part of next patch. |
Reviewers: DavidL, JP ChangeLog: - Convert to optparse for blackberry.py blackberryndk.py scripts Test Cases: - Create sample project using project.py. - Make sure project successfully created. - Make sure blackberry and blackberryndk scripts correctly run the unit testa with -t option. - Make sure created project can be successfully built and run.
|
Updated |
|
@jpl-mac for project.py we need to change it as part of merging from appcelerator to avoid conflict. I'm in the best position to do it this week. For devicemanagement.py I'm changing it as part of TIMOB-9279 |
|
@dlifshitz-maca Thanks for the info, let's keep the communication flowing so the effort is not duplicated (Y) |
| @@ -148,8 +148,7 @@ def __unitTestTraceback(): | |||
|
|
|||
| def __runUnitTests(): | |||
| from tiunittest import UnitTest | |||
|
|
|||
| ndk = None if args.test == True else args.test | |||
| ndk = options.test | |||
There was a problem hiding this comment.
use options.ndk instead
should also be decoded
Reviewers: DavidL, JP ChangeLog: - Convert to optparse for blackberry.py blackberryndk.py scripts Test Cases: - Create sample project using project.py. - Make sure project successfully created. - Make sure blackberry and blackberryndk scripts correctly run the unit testa with -t option. - Make sure created project can be successfully built and run.
|
Updated |
|
Approved |
|
Re-approved |
TIMOB-9309: BlackBerry: Convert argparse to optparse part2
Reviewers: DavidL, JP
ChangeLog:
Test Cases: