Attention is currently required from: dexter, laforge.

Patch set 1:Code-Review +1

View Change

7 comments:

• Patchset:

  • Patch Set #1:

    None of my comments are merge blockers, mostly cosmetics.

• File contrib/csv-encrypt-columns.py:

  • Patch Set #1, Line 29: def dict_keys_to_upper(d: dict):

    typing hint: `-> dict:`

  • Patch Set #1, Line 37: def encrypt_col(self, colname:str, value: str):

    typing hint: `-> Hexstr:`?

  • Patch Set #1, Line 43: def encrypt(self):

    type hint: `-> None:`

  • Patch Set #1, Line 60: parser.add_argument('CSVFILE', help="CSV file name")

    Idea: you can do `type=argparse.FileType('r')`, so that argparse will `open()` the file for you and fail early if it does not exist. Not sure when it gets `close()d` though (perhaps automatically when the script finishes?). As a bonus, a special value `-` will automatically open `stdin`, but in that case the `self.filename + '.encr` logic above will no longer work.

    Maybe generalize this even further, allowing to specify the output file and using `stdin` / `stdout` by default?

    ```
    parser.add_argument('-i', metavar='FILE',
                        type=argparse.FileType('r'),
                        default=sys.stdin,
                        help="Input CSV file to be encrypted")
    parser.add_argument('-o', metavar='FILE',
                        type=argparse.FileType('w'),
                        default=sys.stdout,
                        help="Output CSV file")
    ```

  • Patch Set #1, Line 61: default=[],

    I would remove `default` and put `required=True`, since you require at least one argument.

• File pySim/card_key_provider.py:

  • Patch Set #1, Line 129: str) -> str

    `Hexstr`?

To view, visit change 36930. To unsubscribe, or for help writing mail filters, visit settings.