Adding scalafmt formatting to fmt goal

Adds scalafmt formatting to the fmt command for scala files.

Refactored ScalaFmt class into a base class, with two sub classes ScalaFmtCheckFormat (checks if files are formatted correctly) and ScalaFmtFormat (formats the files).  This ensures that the same version of scalafmt is used for both.

Both of these are currently turned off in pants.ini. Skip=True

Testing Done:
New Integration Test Case
CI passes [#3936](#3963)

Reviewed at https://rbcommons.com/s/twitter/r/4312/

pants.ini

@@ -119,7 +119,9 @@ deps: ["3rdparty:thrift-0.9.2"]
 configuration: %(pants_supportdir)s/checkstyle/coding_style.xml
 
 [compile.scalafmt]
-configuration: %(pants_supportdir)s/scalafmt/config
+skip: True
+
+[fmt.scalafmt]
 skip: True
 
 [compile.findbugs]

src/python/pants/backend/jvm/register.py

@@ -60,7 +60,7 @@
                                                           RunTestJvmPrepCommand)
 from pants.backend.jvm.tasks.scala_repl import ScalaRepl
 from pants.backend.jvm.tasks.scaladoc_gen import ScaladocGen
-from pants.backend.jvm.tasks.scalafmt import ScalaFmt
+from pants.backend.jvm.tasks.scalafmt import ScalaFmtCheckFormat, ScalaFmtFormat
 from pants.backend.jvm.tasks.unpack_jars import UnpackJars
 from pants.build_graph.build_file_aliases import BuildFileAliases
 from pants.goal.goal import Goal
@@ -189,7 +189,8 @@ def register_goals():
   task(name='jvm-dirty', action=JvmRun, serialize=False).install('run-dirty')
   task(name='scala', action=ScalaRepl, serialize=False).install('repl')
   task(name='scala-dirty', action=ScalaRepl, serialize=False).install('repl-dirty')
-  task(name='scalafmt', action=ScalaFmt, serialize=False).install('compile', first=True)
+  task(name='scalafmt', action=ScalaFmtCheckFormat, serialize=False).install('compile', first=True)
+  task(name='scalafmt', action=ScalaFmtFormat, serialize=False).install('fmt')
   task(name='test-jvm-prep-command', action=RunTestJvmPrepCommand).install('test', first=True)
   task(name='binary-jvm-prep-command', action=RunBinaryJvmPrepCommand).install('binary', first=True)
   task(name='compile-jvm-prep-command', action=RunCompileJvmPrepCommand).install('compile', first=True)

src/python/pants/backend/jvm/tasks/BUILD

@@ -620,6 +620,7 @@ python_library(
     'src/python/pants/base:exceptions',
     'src/python/pants/build_graph',
     'src/python/pants/option',
+    'src/python/pants/util:meta',
   ]
 )
 

src/python/pants/backend/jvm/tasks/scalafmt.py

@@ -5,15 +5,21 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+from abc import abstractproperty
+
 from pants.backend.jvm.targets.jar_dependency import JarDependency
 from pants.backend.jvm.tasks.nailgun_task import NailgunTask
 from pants.base.exceptions import TaskError
 from pants.build_graph.target import Target
 from pants.option.custom_types import file_option
+from pants.util.meta import AbstractClass
+
 
+class ScalaFmt(NailgunTask, AbstractClass):
+  """Abstract class to run ScalaFmt commands.
 
-class ScalaFmt(NailgunTask):
-  """Checks Scala code matches scalafmt style.
+  Classes that inherit from this should override get_command_args and
+  process_results to run different scalafmt commands
 
   :API: public
   """
@@ -23,8 +29,8 @@ class ScalaFmt(NailgunTask):
   @classmethod
   def register_options(cls, register):
     super(ScalaFmt, cls).register_options(register)
-    register('--skip', type=bool, fingerprint=True, help='Skip Scalafmt Check')
-    register('--configuration', advanced=True, type=file_option, fingerprint=True,
+    register('--skip', type=bool, fingerprint=False, help='Skip Scalafmt Check')
+    register('--configuration', advanced=True, type=file_option, fingerprint=False,
               help='Path to scalafmt config file, if not specified default scalafmt config used')
     cls.register_jvm_tool(register,
                           'scalafmt',
@@ -45,24 +51,29 @@ def execute(self):
     if sources:
       files = ",".join(sources)
 
-      config_file = self.get_options().configuration
-
-      """If no config file is specified use default scalafmt config"""
-      args = ['--test', '--files', files]
-      if config_file != None:
-        args.extend(['--config', config_file])
-
       result = self.runjava(classpath=self.tool_classpath('scalafmt'),
                    main=self._SCALAFMT_MAIN,
-                   args=args,
+                   args=self.get_command_args(files),
                    workunit_name='scalafmt')
 
-      if result != 0:
-        scalafmt_cmd = 'scalafmt -i --files {}'.format(files)
-        if config_file != None:
-          scalafmt_cmd += ' --config {}'.format(config_file)
-
-        raise TaskError('Scalafmt failed with exit code {} to fix run: `{}`'.format(result, scalafmt_cmd), exit_code=result)
+      self.process_results(result)
+
+  @abstractproperty
+  def get_command_args(self, files):
+    """Returns the arguments used to run Scalafmt command.
+
+    The return value should be an array of strings.  For
+    example, to run the Scalafmt help command:
+    ['--help']
+    """
+
+  @abstractproperty
+  def process_results(self, result):
+    """This method processes the results of the scalafmt command.
+
+    No return value is expected.  If an error occurs running
+    Scalafmt raising a TaskError is recommended.
+    """
 
   def get_non_synthetic_scala_targets(self, targets):
     return filter(
@@ -77,3 +88,53 @@ def calculate_sources(self, targets):
       sources.update(source for source in target.sources_relative_to_buildroot()
                       if source.endswith(self._SCALA_SOURCE_EXTENSION))
     return sources
+
+
+class ScalaFmtCheckFormat(ScalaFmt):
+  """This Task checks that all scala files in the target are formatted
+  correctly.
+
+  If the files are not formatted correctly an error is raised
+  including the command to run to format the files correctly
+
+  :API: public
+  """
+
+  def get_command_args(self, files):
+    # If no config file is specified use default scalafmt config.
+    config_file = self.get_options().configuration
+    args = ['--test', '--files', files]
+    if config_file!= None:
+      args.extend(['--config', config_file])
+
+    return args
+
+  def process_results(self, result):
+    if result != 0:
+      raise TaskError('Scalafmt failed with exit code {} to fix run: `./pants fmt <targets>` config{}'.format(result, self.get_options().configuration), exit_code=result)
+
+
+class ScalaFmtFormat(ScalaFmt):
+  """This Task reads all scala files in the target and emits
+  the source in a standard style as specified by the configuration
+  file.
+
+  This task mutates the underlying flies.
+
+  :API: public
+  """
+
+  def get_command_args(self, files):
+    # If no config file is specified use default scalafmt config.
+    config_file = self.get_options().configuration
+    args = ['-i', '--files', files]
+    if config_file!= None:
+      args.extend(['--config', config_file])
+
+    return args
+
+  def process_results(self, result):
+    # Processes the results of running the scalafmt command.
+    if result != 0:
+      raise TaskError('Scalafmt failed to format files', exit_code=result)
+

tests/python/pants_test/backend/jvm/tasks/BUILD

@@ -594,6 +594,7 @@ python_tests(
     'src/python/pants/backend/jvm/tasks:scalafmt',
     'tests/python/pants_test:int-test',
   ],
+  timeout=120,
   tags = {'integration'},
 )
 

tests/python/pants_test/backend/jvm/tasks/test_scalafmt.py

@@ -12,18 +12,53 @@
 
 
 class ScalaFmtIntegrationTests(PantsRunIntegrationTest):
-  def test_scalafmt_fail(self):
+  def test_scalafmt_fail_default_config(self):
     target = '{}/badscalastyle::'.format(TEST_DIR)
     # test should fail because of style error.
     failing_test = self.run_pants(['compile', target],
-      {'compile.scalafmt':{'skip': 'False'}})
+      {'compile.scalafmt':{'skip':'False'}})
+    self.assert_failure(failing_test)
 
+  def test_scalafmt_fail(self):
+    target = '{}/badscalastyle::'.format(TEST_DIR)
+    # test should fail because of style error.
+    failing_test = self.run_pants(['compile', target],
+      {'compile.scalafmt':{'skip':'False',
+      'configuration':'%(pants_supportdir)s/scalafmt/config'}})
     self.assert_failure(failing_test)
 
   def test_scalafmt_disabled(self):
     target = '{}/badscalastyle::'.format(TEST_DIR)
     # test should pass because of scalafmt disabled.
     failing_test = self.run_pants(['compile', target],
-      {'compile.scalafmt':{'skip': 'True'}})
-
+      {'compile.scalafmt': {'skip':'True'}})
     self.assert_success(failing_test)
+
+  def test_scalafmt_format_default_config(self):
+    self.format_file_and_verify_fmt({'skip':'False'})
+
+  def test_scalafmt_format(self):
+    self.format_file_and_verify_fmt({'skip':'False',
+      'configuration':'%(pants_supportdir)s/scalafmt/config'})
+
+  def format_file_and_verify_fmt(self, options):
+    # take a snapshot of the file which we can write out
+    # after the test finishes executing.
+    test_file_name = '{}/badscalastyle/BadScalaStyle.scala'.format(TEST_DIR)
+    f = open(test_file_name, 'r')
+    contents = f.read()
+    f.close()
+
+    # format an incorrectly formatted file.
+    target = '{}/badscalastyle::'.format(TEST_DIR)
+    fmt_result = self.run_pants(['fmt', target], {'fmt.scalafmt':options})
+    self.assert_success(fmt_result)
+
+    # verify that the compile check not passes.
+    test_fmt = self.run_pants(['compile', target], {'compile.scalafmt':options})
+    self.assert_success(test_fmt)
+
+    # restore the file to its original state.
+    f = open(test_file_name, 'w')
+    f.write(contents)
+    f.close()