Skip to content

Commit

Permalink
Fix #199: Correctly checking _inherited_absl_flags by comparing to …
Browse files Browse the repository at this point in the history
…`None` instead of implicit bool.

The documented contract is to skip absl flags when `inherited_absl_flags` is passed `None`. A `FlagValues` instance without any flags evaluates to `False` and shouldn't let it skip.

PiperOrigin-RevId: 578025536
  • Loading branch information
yilei authored and Copybara-Service committed Oct 31, 2023
1 parent 0ff1e24 commit 7df2f42
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 4 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com).
tests equality of `dataclass.dataclass` objects with better error messages
when the assert fails.

### Changed

* (flags) `absl.flags.argparse_flags.ArgumentParser` now correctly inherits
an empty instance of `FlagValues` and ensure absl flags, such as
`--flagfile`, `--undefok` are supported.

### Fixed

* The flag `foo` no longer retains the value `bar` after `FLAGS.foo = bar`
Expand Down
8 changes: 4 additions & 4 deletions absl/flags/argparse_flags.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,15 +145,15 @@ def __init__(self, **kwargs):
'--helpfull', action=_HelpFullAction,
default=argparse.SUPPRESS, help='show full help message and exit')

if self._inherited_absl_flags:
if self._inherited_absl_flags is not None:
self.add_argument(
'--undefok', default=argparse.SUPPRESS, help=argparse.SUPPRESS)
self._define_absl_flags(self._inherited_absl_flags)

def parse_known_args(self, args=None, namespace=None):
if args is None:
args = sys.argv[1:]
if self._inherited_absl_flags:
if self._inherited_absl_flags is not None:
# Handle --flagfile.
# Explicitly specify force_gnu=True, since argparse behaves like
# gnu_getopt: flags can be specified after positional arguments.
Expand All @@ -172,7 +172,7 @@ def parse_known_args(self, args=None, namespace=None):
if undefok is not undefok_missing:
namespace.undefok = undefok

if self._inherited_absl_flags:
if self._inherited_absl_flags is not None:
# Handle --undefok. At this point, `args` only contains unknown flags,
# so it won't strip defined flags that are also specified with --undefok.
# For Python <= 2.7.8: https://bugs.python.org/issue9351, a bug where
Expand Down Expand Up @@ -350,7 +350,7 @@ def __call__(self, parser, namespace, values, option_string=None):
parser.print_help()

absl_flags = parser._inherited_absl_flags # pylint: disable=protected-access
if absl_flags:
if absl_flags is not None:
modules = sorted(absl_flags.flags_by_module_dict())
main_module = sys.argv[0]
if main_module in modules:
Expand Down
15 changes: 15 additions & 0 deletions absl/flags/tests/argparse_flags_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,21 @@ def test_argument_default(self):
args = parser.parse_args([])
self.assertEqual(args.magic_number, 23)

def test_empty_inherited_absl_flags(self):
parser = argparse_flags.ArgumentParser(
inherited_absl_flags=flags.FlagValues()
)
parser.add_argument('--foo')
flagfile = self.create_tempfile(content='--foo=bar').full_path
# Make sure these flags are still available when inheriting an empty
# FlagValues instance.
ns = parser.parse_args([
'--undefok=undefined_flag',
'--undefined_flag=value',
'--flagfile=' + flagfile,
])
self.assertEqual(ns.foo, 'bar')


class ArgparseWithAppRunTest(parameterized.TestCase):

Expand Down

0 comments on commit 7df2f42

Please sign in to comment.