Skip to content

Commit

Permalink
Set the fromfile option arg's default to True (#7513)
Browse files Browse the repository at this point in the history
There's no reason not to support this for all options.
If this goes smoothly a future change will deprecate
the fromfile arg.
  • Loading branch information
benjyw authored Apr 10, 2019
1 parent eebf02f commit 65ed665
Show file tree
Hide file tree
Showing 9 changed files with 29 additions and 48 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def register_options(cls, register):
'memory, (re-)generate them on disk using the installed Go BUILD file template.')

# TODO(John Sirois): Add docs for the template parameters.
register('--template', metavar='<template>', fromfile=True,
register('--template', metavar='<template>',
default=cls._default_template(),
advanced=True, fingerprint=True,
help='A Go BUILD file mustache template to use with --materialize.')
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/build_graph/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ class TagAssignments(Subsystem):

@classmethod
def register_options(cls, register):
register('--tag-targets-mappings', type=dict, default=None, fromfile=True, fingerprint=True,
register('--tag-targets-mappings', type=dict, default=None, fingerprint=True,
help='Dict with tag assignments for targets. Ex: { "tag1": ["path/to/target:foo"] }')

@classmethod
Expand Down
6 changes: 2 additions & 4 deletions src/python/pants/help/help_formatter.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,9 @@ def format_option(self, ohi):
"""
lines = []
choices = 'one of: [{}] '.format(ohi.choices) if ohi.choices else ''
arg_line = ('{args} {fromfile}{dflt}'
arg_line = ('{args} {dflt}'
.format(args=self._maybe_cyan(', '.join(ohi.display_args)),
dflt=self._maybe_green('({}default: {})'.format(choices, ohi.default)),
fromfile=self._maybe_green('(@fromfile value supported) ' if ohi.fromfile
else '')))
dflt=self._maybe_green('({}default: {})'.format(choices, ohi.default))))
lines.append(arg_line)

indent = ' '
Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/help/help_info_extracter.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

class OptionHelpInfo(namedtuple('_OptionHelpInfo',
['registering_class', 'display_args', 'scoped_cmd_line_args', 'unscoped_cmd_line_args',
'typ', 'fromfile', 'default', 'help', 'deprecated_message', 'removal_version',
'typ', 'default', 'help', 'deprecated_message', 'removal_version',
'removal_hint', 'choices'])):
"""A container for help information for a single option.
Expand All @@ -27,7 +27,6 @@ class OptionHelpInfo(namedtuple('_OptionHelpInfo',
unscoped_cmd_line_args: The unscoped raw flag names allowed on the cmd line in this option's
scope context (e.g., [--baz, --no-baz, --qux])
typ: The type of the option.
fromfile: `True` if the option supports @fromfile value loading.
default: The value of this option if no flags are specified (derived from config and env vars).
help: The help message registered for this option.
deprecated_message: If deprecated: A message explaining that this option is deprecated at
Expand Down Expand Up @@ -183,7 +182,6 @@ def get_option_help_info(self, args, kwargs):
scoped_cmd_line_args=scoped_cmd_line_args,
unscoped_cmd_line_args=unscoped_cmd_line_args,
typ=typ,
fromfile=kwargs.get('fromfile', False),
default=default,
help=help_msg,
deprecated_message=deprecated_message,
Expand Down
10 changes: 5 additions & 5 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,13 @@ def register_bootstrap_options(cls, register):
advanced=True,
help='Verify that all config file values correspond to known options.')

register('--build-ignore', advanced=True, type=list, fromfile=True,
register('--build-ignore', advanced=True, type=list,
default=['.*/', default_rel_distdir, 'bower_components/',
'node_modules/', '*.egg-info/'],
help='Paths to ignore when identifying BUILD files. '
'This does not affect any other filesystem operations. '
'Patterns use the gitignore pattern syntax (https://git-scm.com/docs/gitignore).')
register('--pants-ignore', advanced=True, type=list, fromfile=True,
register('--pants-ignore', advanced=True, type=list,
default=['.*/', default_rel_distdir],
help='Paths to ignore for all filesystem operations performed by pants '
'(e.g. BUILD file scanning, glob matching, etc). '
Expand All @@ -231,10 +231,10 @@ def register_bootstrap_options(cls, register):

register('--exclude-target-regexp', advanced=True, type=list, default=[], daemon=False,
metavar='<regexp>', help='Exclude target roots that match these regexes.')
register('--subproject-roots', type=list, advanced=True, fromfile=True, default=[],
register('--subproject-roots', type=list, advanced=True, default=[],
help='Paths that correspond with build roots for any subproject that this '
'project depends on.')
register('--owner-of', type=list, default=[], daemon=False, fromfile=True, metavar='<path>',
register('--owner-of', type=list, default=[], daemon=False, metavar='<path>',
help='Select the targets that own these files. '
'This is the third target calculation strategy along with the --changed-* '
'options and specifying the targets directly. These three types of target '
Expand Down Expand Up @@ -294,7 +294,7 @@ def register_bootstrap_options(cls, register):
'signal to the remote pantsd-runner process before killing it.')
register('--pantsd-log-dir', advanced=True, default=None,
help='The directory to log pantsd output to.')
register('--pantsd-invalidation-globs', advanced=True, type=list, fromfile=True, default=[],
register('--pantsd-invalidation-globs', advanced=True, type=list, default=[],
help='Filesystem events matching any of these globs will trigger a daemon restart.')

# Watchman options.
Expand Down
13 changes: 7 additions & 6 deletions src/python/pants/option/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -456,12 +456,13 @@ def to_value_type(val_str):

# Helper function to expand a fromfile=True value string, if needed.
# May return a string or a dict/list decoded from a json/yaml file.
def expand(val_str):
if kwargs.get('fromfile', False) and val_str and val_str.startswith('@'):
if val_str.startswith('@@'): # Support a literal @ for fromfile values via @@.
return val_str[1:]
def expand(val_or_str):
if (kwargs.get('fromfile', True) and val_or_str and
isinstance(val_or_str, str) and val_or_str.startswith('@')):
if val_or_str.startswith('@@'): # Support a literal @ for fromfile values via @@.
return val_or_str[1:]
else:
fromfile = val_str[1:]
fromfile = val_or_str[1:]
try:
with open(fromfile, 'r') as fp:
s = fp.read().strip()
Expand All @@ -475,7 +476,7 @@ def expand(val_str):
raise self.FromfileError('Failed to read {} in {} from file {}: {}'.format(
dest, self._scope_str(), fromfile, e))
else:
return val_str
return val_or_str

# Get value from config files, and capture details about its derivation.
config_details = None
Expand Down
6 changes: 1 addition & 5 deletions tests/python/pants_test/help/test_help_formatter.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class OptionHelpFormatterTest(unittest.TestCase):
def format_help_for_foo(self, **kwargs):
ohi = OptionHelpInfo(registering_class=type(None), display_args=['--foo'],
scoped_cmd_line_args=['--foo'], unscoped_cmd_line_args=['--foo'],
typ=bool, fromfile=False, default=None, help='help for foo',
typ=bool, default=None, help='help for foo',
deprecated_message=None, removal_version=None, removal_hint=None,
choices=None)
ohi = ohi._replace(**kwargs)
Expand All @@ -28,10 +28,6 @@ def test_format_help(self):
line = self.format_help_for_foo(default='MYDEFAULT')
self.assertEqual('--foo (default: MYDEFAULT)', line)

def test_format_help_fromfile(self):
line = self.format_help_for_foo(fromfile=True)
self.assertEqual('--foo (@fromfile value supported) (default: None)', line)

def test_suppress_advanced(self):
args = ['--foo']
kwargs = {'advanced': True}
Expand Down
12 changes: 0 additions & 12 deletions tests/python/pants_test/help/test_help_info_extracter.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,6 @@ def test_deprecated(self):
self.assertEqual('do not use this', ohi.removal_hint)
self.assertIsNotNone(ohi.deprecated_message)

def test_fromfile(self):
ohi = HelpInfoExtracter('').get_option_help_info([], {})
self.assertFalse(ohi.fromfile)

kwargs = {'fromfile': False}
ohi = HelpInfoExtracter('').get_option_help_info([], kwargs)
self.assertFalse(ohi.fromfile)

kwargs = {'fromfile': True}
ohi = HelpInfoExtracter('').get_option_help_info([], kwargs)
self.assertTrue(ohi.fromfile)

def test_grouping(self):
def do_test(kwargs, expected_basic=False, expected_recursive=False, expected_advanced=False):
def exp_to_len(exp):
Expand Down
22 changes: 11 additions & 11 deletions tests/python/pants_test/option/test_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,11 @@ def register_global(*args, **kwargs):
options.register('scoped.and-dashed', '--spam')

# For fromfile test
options.register('fromfile', '--string', fromfile=True)
options.register('fromfile', '--intvalue', type=int, fromfile=True)
options.register('fromfile', '--dictvalue', type=dict, fromfile=True)
options.register('fromfile', '--listvalue', type=list, fromfile=True)
options.register('fromfile', '--appendvalue', type=list, member_type=int, fromfile=True)
options.register('fromfile', '--string')
options.register('fromfile', '--intvalue', type=int)
options.register('fromfile', '--dictvalue', type=dict)
options.register('fromfile', '--listvalue', type=list)
options.register('fromfile', '--appendvalue', type=list, member_type=int)

# For fingerprint tests
options.register('fingerprinting', '--inverted') # Implicitly: daemon=True
Expand Down Expand Up @@ -1041,8 +1041,8 @@ def test_middle_scoped_options(self):

options = self._parse('./pants',
env={
'PANTS_A': 100,
'PANTS_COMPILE_A': 99})
'PANTS_A': '100',
'PANTS_COMPILE_A': '99'})
self.assertEqual(100, options.for_global_scope().a)
self.assertEqual(99, options.for_scope('compile').a)
self.assertEqual(99, options.for_scope('compile.java').a)
Expand All @@ -1058,7 +1058,7 @@ def test_middle_scoped_options(self):

# Command line has precedence over environment.
options = self._parse('./pants compile --a=99',
env={'PANTS_A': 100}, )
env={'PANTS_A': '100'}, )
self.assertEqual(100, options.for_global_scope().a)
self.assertEqual(99, options.for_scope('compile').a)
self.assertEqual(99, options.for_scope('compile.java').a)
Expand All @@ -1068,14 +1068,14 @@ def test_middle_scoped_options(self):
config={
'DEFAULT': {'a': 100},
},
env={'PANTS_COMPILE_A': 99}, )
env={'PANTS_COMPILE_A': '99'}, )
self.assertEqual(100, options.for_global_scope().a)
self.assertEqual(99, options.for_scope('compile').a)
self.assertEqual(99, options.for_scope('compile.java').a)

# Command line global overrides the middle scope setting in then env.
options = self._parse('./pants --a=100',
env={'PANTS_COMPILE_A': 99}, )
env={'PANTS_COMPILE_A': '99'}, )
self.assertEqual(100, options.for_global_scope().a)
self.assertEqual(100, options.for_scope('compile').a)
self.assertEqual(100, options.for_scope('compile.java').a)
Expand All @@ -1094,7 +1094,7 @@ def test_middle_scoped_options(self):
config={
'compile': {'a': 99},
},
env={'PANTS_A': 100}, )
env={'PANTS_A': '100'}, )
self.assertEqual(100, options.for_global_scope().a)
self.assertEqual(100, options.for_scope('compile').a)
self.assertEqual(100, options.for_scope('compile.java').a)
Expand Down

0 comments on commit 65ed665

Please sign in to comment.