From 65ed6651419c0baed7cb8a4c6114aa202322be8d Mon Sep 17 00:00:00 2001
From: Benjy Weinberger <benjyw@gmail.com>
Date: Wed, 10 Apr 2019 07:47:28 +0300
Subject: [PATCH] Set the fromfile option arg's default to True (#7513)

There's no reason not to support this for all options.
If this goes smoothly a future change will deprecate
the fromfile arg.
---
 .../pants/contrib/go/tasks/go_buildgen.py     |  2 +-
 src/python/pants/build_graph/target.py        |  2 +-
 src/python/pants/help/help_formatter.py       |  6 ++---
 src/python/pants/help/help_info_extracter.py  |  4 +---
 src/python/pants/option/global_options.py     | 10 ++++-----
 src/python/pants/option/parser.py             | 13 ++++++-----
 .../pants_test/help/test_help_formatter.py    |  6 +----
 .../help/test_help_info_extracter.py          | 12 ----------
 .../python/pants_test/option/test_options.py  | 22 +++++++++----------
 9 files changed, 29 insertions(+), 48 deletions(-)

diff --git a/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py b/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py
index 9ce9c083536..d9f89579d33 100644
--- a/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py
+++ b/contrib/go/src/python/pants/contrib/go/tasks/go_buildgen.py
@@ -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.')
diff --git a/src/python/pants/build_graph/target.py b/src/python/pants/build_graph/target.py
index 33fa43d966d..6d98a331002 100644
--- a/src/python/pants/build_graph/target.py
+++ b/src/python/pants/build_graph/target.py
@@ -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
diff --git a/src/python/pants/help/help_formatter.py b/src/python/pants/help/help_formatter.py
index a40bac9cef1..14e0d0b4af2 100644
--- a/src/python/pants/help/help_formatter.py
+++ b/src/python/pants/help/help_formatter.py
@@ -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 = '    '
diff --git a/src/python/pants/help/help_info_extracter.py b/src/python/pants/help/help_info_extracter.py
index a4718fa6bb1..d45c22984b9 100644
--- a/src/python/pants/help/help_info_extracter.py
+++ b/src/python/pants/help/help_info_extracter.py
@@ -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.
 
@@ -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
@@ -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,
diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py
index 687dda4322f..e86c9cefa10 100644
--- a/src/python/pants/option/global_options.py
+++ b/src/python/pants/option/global_options.py
@@ -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). '
@@ -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 '
@@ -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.
diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py
index a0ff9b0c6e3..3267083ea77 100644
--- a/src/python/pants/option/parser.py
+++ b/src/python/pants/option/parser.py
@@ -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()
@@ -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
diff --git a/tests/python/pants_test/help/test_help_formatter.py b/tests/python/pants_test/help/test_help_formatter.py
index 4d03e519367..a63a0ec488b 100644
--- a/tests/python/pants_test/help/test_help_formatter.py
+++ b/tests/python/pants_test/help/test_help_formatter.py
@@ -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)
@@ -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}
diff --git a/tests/python/pants_test/help/test_help_info_extracter.py b/tests/python/pants_test/help/test_help_info_extracter.py
index 961b40d6741..2b4768403ca 100644
--- a/tests/python/pants_test/help/test_help_info_extracter.py
+++ b/tests/python/pants_test/help/test_help_info_extracter.py
@@ -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):
diff --git a/tests/python/pants_test/option/test_options.py b/tests/python/pants_test/option/test_options.py
index 0ef82115393..b720edf596c 100644
--- a/tests/python/pants_test/option/test_options.py
+++ b/tests/python/pants_test/option/test_options.py
@@ -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
@@ -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)
@@ -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)
@@ -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)
@@ -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)