[tor-bugs] #9975 [Flashproxy]: use argparse rather than getopt

[Tor Bug Tracker & Wiki]

#9975: use argparse rather than getopt
-----------------------------+--------------------------
     Reporter:  infinity0    |      Owner:  infinity0
         Type:  enhancement  |     Status:  needs_review
     Priority:  minor        |  Milestone:
    Component:  Flashproxy   |    Version:
   Resolution:               |   Keywords:
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+--------------------------

Comment (by dcf):

 Thank you for rewriting your branch as you have done. There is good stuff
 here, though I still have reservations.

 [https://github.com/infinity0/flashproxy/commit/5445680be0ba2273cec01f6fab91ef48ac47c86a
 5445680b],
 [https://github.com/infinity0/flashproxy/commit/8b1c987a05d8031d0670e97e98f36083dd5b5a25
 8b1c987a], and
 [https://github.com/infinity0/flashproxy/commit/ea156f7066e9856044a02f6120db7fbbafe28096
 ea156f70] are great, go ahead.

 [https://github.com/infinity0/flashproxy/commit/95c4a889b6eca77d48ac4c9f9df8acf308800a9c
 95c4a889] and
 [https://github.com/infinity0/flashproxy/commit/c6650817a6046c80aa4296f13ea2f3fc4a94e2c7
 c6650817] are okay. I'm fine with those being merged any time. I hate that
 it will permanently break some command lines that currently work--I just
 know I'll be bitten by this fact in the future, and will curse myself for
 having allowed it. But I'm willing to try and ignore that feeling if you
 feel strongly for argparse.

 I strongly disagree with the localization of global variables in
 [https://github.com/infinity0/flashproxy/commit/913fa0cc12024aef4d6052443df4d15d1e79a76e
 913fa0cc]
 [https://github.com/infinity0/flashproxy/commit/116582c93981b94659872cc7cf3f6ee11de74fe1
 116582c9]
 [https://github.com/infinity0/flashproxy/commit/817a00e9bab8efe28beb90d4d07f1ab84f0bcb40
 817a00e9]. Options are global state. They should be represented by global
 variables. Is there a technical reason for localizing the variables,
 having to do with argparse? Is it to avoid copying values from `ns` to
 `options`?

 I've read the code of `add_module_opts` many times and I still don't
 understand what it's doing. Is there any way to rewrite it so that it's
 more clear what is going on? I don't see why the code has do anything
 other than call `parser.add_argument` a bunch of times.
 {{{
 +    old_parse = parser.parse_args
 +    def parse_args(namespace):
 +        options.transport = namespace.transport
 +        options.facilitator_pubkey = namespace.facilitator_pubkey
 +        return namespace
 +    parser.parse_args = lambda *a, **kw: parse_args(old_parse(*a, **kw))
 }}}

 I think you will agree that `ignore_pubkey` is ugly. It's contrary to the
 purpose of this branch, which is greater orthogonality and fewer special
 cases. Do you have a second or third proposed alternate design?
 {{{
 +def add_module_opts(parser, ignore_pubkey=False):
  ...
 +    parser.add_argument("--facilitator-pubkey", metavar="FILENAME",
 +        help=(_OPTION_IGNORED if ignore_pubkey else "encrypt
 registrations to "
 +        "the given PEM-formatted public key file (default built-in)."))
 }}}

 I hate to be so negative. I've tried to get over my misgivings about this
 branch but I can't quite shake them.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9975#comment:14>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online