Refactor Mach Mig generation scripts. [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 6 comments![]( "Open in Gerrit")
def _generate_and_fix(user_c, server_c, user_h, server_h, defs, sdk, clang_path,Force callers to provide everything as a keyword argument, like in the other file: `_generate_and_fix(*, user_c, server_c, …)`.
(if you have a reason to want to leave some of the arguments as not keyword-only, that’s probably OK too, but I definitely don’t think that all of them should be allowed to be non-keyword.)
def _generate_and_fix(user_c, server_c, user_h, server_h, defs, sdk, clang_path,
mig_path, migcom_path, arch, extra_args):These are probably just as optional here as they are in the other file. `sdk=None, clang_path=None, …`
mig_path, migcom_path, arch, extra_args):`mig_args`?
contents += open(file, 'r').read()Slightly better with a `with` context manager, to ensure precise timing of the file close when the file is no longer needed.
return parser.parse_known_args(args)`parse_known_args` is too loosey-goosey. I don’t like it because it masks errors and it’s not resilient in the face of future changes to the script and the set of arguments that it understands.
Preferable:
```python
parser.add_argument('mig_args', nargs='*')
return parser.parse_args(args)
```
Then, use the magic `--` argument to separate the required arguments above this line from `mig_args`. This allows option-style arguments that begin with `-` to make it through to `mig_args` without becoming argparse errors due to not being defined in `parser`.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 6 comments![]( "Open in Gerrit")
def _generate_and_fix(user_c, server_c, user_h, server_h, defs, sdk, clang_path,Force callers to provide everything as a keyword argument, like in the other file: `_generate_and_fix(*, user_c, server_c, …)`.
(if you have a reason to want to leave some of the arguments as not keyword-only, that’s probably OK too, but I definitely don’t think that all of them should be allowed to be non-keyword.)
Done
def _generate_and_fix(user_c, server_c, user_h, server_h, defs, sdk, clang_path,
mig_path, migcom_path, arch, extra_args):These are probably just as optional here as they are in the other file. `sdk=None, clang_path=None, …`
Done
mig_path, migcom_path, arch, extra_args):Justin Cohen`mig_args`?
Done
Slightly better with a `with` context manager, to ensure precise timing of the file close when the file is no longer needed.
Done
`parse_known_args` is too loosey-goosey. I don’t like it because it masks errors and it’s not resilient in the face of future changes to the script and the set of arguments that it understands.
Preferable:
```python
parser.add_argument('mig_args', nargs='*')
return parser.parse_args(args)
```Then, use the magic `--` argument to separate the required arguments above this line from `mig_args`. This allows option-style arguments that begin with `-` to make it through to `mig_args` without becoming argparse errors due to not being defined in `parser`.
Done -- note to make nargs='*', I had to reorder things to be:
"%(prog)s [OPTIONS] <defs> <user_c> <server_c> <user_h> <server_h> -- [mig arguments]"
And I updated the ArgumentParser comment to reflect that.
PTAL!
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |