[PATCH] D25138: [LIT] Add option to require PRs for XFAIL directives

[Brian Gesiak via llvm-commits]

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

I agree with @ddunbar's suggestion on the syntax.



> main.py:276
>                        action="store_false", default=True)
> +    group.add_option("", "--xfail-requires-pr", dest="xFailRequirePR",
> +                      help="Require PRs for XFAIL tests",

Looks like this will have to be rebased -- these have since been migrated to `argparse` and `add_argument` in https://reviews.llvm.org/rL283152.

I agree with @ddunbar's suggestion: specifying `--bug-tracker-prefix` should imply `--xfail-requires-pr`, which would eliminate the need for this option.

nit-pick: Modern Python's naming convention for variables is `snake_case` (https://www.python.org/dev/peps/pep-0008/#function-names), which would make this variable name `xfail_requires_pr`. One nice part about `argparse` is that `foo-bar` is automatically converted to an attribute named `foo_bar`, so we could eliminate the `dest=` parameter here. Still, I guess there's little value in migrating this codebase from `mixedCase` to `snake_case`, whether incrementally or all at once. But I was wondering if maybe @ddunbar has any thoughts here?

> main.py:277
> +    group.add_option("", "--xfail-requires-pr", dest="xFailRequirePR",
> +                      help="Require PRs for XFAIL tests",
> +                      action="store_true", default=False)

nit-pick: If we use "PR" in the help messages for `lit`, it'd be great to spell out "problem report" in at least one spot in the help messages. I think there's room for confusion, since GitHub users are used to "PR" meaning "pull request".

Related: https://reviews.llvm.org/D25331

> main.py:282
> +                      help=("Prefixes for specifying bugs in XFAIL directives "
> +                            "(defaults to include http://llvm.org/PR)"),
> +                      type=str, action="append", default=['http://llvm.org/PR'])

You can use `%(default)s` instead of writing out the default value. `argparse` will replace it with the value specified in the `default=` parameter.

https://reviews.llvm.org/D25138