Adding scalafmt formatting to fmt goal

Review Request #4312 — Created Oct. 14, 2016 and submitted

caitiem20
pants
pants-reviews
ity, kwlzn, mateor, stuhood

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

New Integration Test Case
CI passes #3936

  • 0
  • 0
  • 5
  • 2
  • 7
Description From Last Updated
  1. Thanks a lot!

  2. src/python/pants/backend/jvm/tasks/scalafmt.py (Diff revision 1)
     
     
     
     

    pythondocs for Tasks are parsed and included in their help strings: the first line should be a complete sentence giving a summary of the rest. Then, the remainder of the description should follow a blank line. Example:

    """ScalaFmt base class executes the help command.
    
    Classes that inherit from this should override get_command_args and
    process_results to run different scalafmt commands
    
    :API: public
    """
    

    This also goes for any other block comments.

  3. I think that these can probably both default to skip=False now (and you can continue to override the default in pantsbuild/pants's own pants.ini).

  4. Recommend making the base class abstract by extending AbstractClass, and then marking this method @abstractmethod: good example here: https://github.com/pantsbuild/pants/blob/d30cca1e0ecb9cc0e1b7e2cd0ff6e7e077e62a52/src/python/pants/base/build_file_target_factory.py

    1. Done! Agree much better, wasn't sure if you guys @abstractmethod or not, thanks!

  5. ditto @abstractmethod

  6. Ditto "single sentence description on first line"

  7. Should probably make it more explicit that this mutates the sources.

  8. 
      
  1. Ship It!
  2. 
      
  1. Not directly related to this change, but should we combine all linting (e.g., checkstyle) and formatting (e.g., scalafmt and isort) into a 'lint' goal? Seems more intuitive than 'fmt' (unless you come from the Go world).

    1. Putting both sets of tasks in the same goal would cause them to always run together, which would not be desirable. As it stands, style/linting run during compile, and formatting is an explicit separate goal.

    2. Yeah, but linting on every compile can be onerous. I find it very annoying to have a test run fail on lint errors during development. Lint isn't important until you're ready to commit.

    3. At twitter we move all of the linting tasks to the end of test... would love to do that upstream if people were amenable.

    4. Why not just lint?

    5. Because the most common thing that people do locally is run ./pants test, and having that agree with what CI runs is beneficial: when the default thing that people run locally disagrees with what gets run remotely, it causes friction. An alternative would be to retrain everyone to run ./pants test lint locally, or give ./pants lint a dep on test, and then retrain people to run that.

      So moving lint tasks to the end of test is mostly about 1) alignment between CI and local, 2) while not blocking people from running their tests.

    6. Or just use the product-based scheduling I already inlined on the original CR (or something like it), which forces it {fmt, lint, whatever} to run before the compile phase. The issue was dropped as wontfix - but the code is here: https://rbcommons.com/s/twitter/r/4216/#comment447997

      I am not going to block anything you truly want to ship, but linting or fmt after compile makes this hard to use, imo.

      Unless you have a workflow that I am misunderstanding - doesn't linting after test just churn fingerprint/dev time? Say I edit a common library, I compile the world and then BAM lint errors. So the workflow is to then edit and wait through a recompile?

      I don't see why you wouldn't just gate compile on a passed lint and give that feedback ~immediately.

    7. I hadn't realized that your comment was a response to Benjy, and just now read his post. I also hate linting errors during development, so that seems reasonable enough.

      Perhaps add an optional product requirement to JUnit for 'fmtted' code and install the fmt within or before codegen? I think that would skip the compile churn while also avoiding development pain or retraining.

      A stickier problem with all the competing workflows concerns laid out. Will consider for a bit, since this landed as-is.

    8. @Stu Interesting point re: friction. I think the biggest downside I see to bundling test+lint (or any big rock-ish goals, for that matter) is that in order to normalize expectations, you end up sacrificing some degree of flexibility/speed for your "power users" and creating another sort of friction (cognitive load) where users get dinged for things they didn't explicitly ask the tool to do yet. I think this can actually be more frustrating/harmful in certain cases for users that would prefer pants to behave logically/literally (e.g. "when I say A, run A - not A, B, C") and to be able to compose as needed instead. I can also see the benefit of the "batteries included" approach - and while it makes sense, it's also a tradeoff that squarely blocks those in the "compose" camp. I also think it adds slight bloat which could harm our goals around performance/lightning fast tooling where ./pants test is now the cumulative cost of all of its inner goals.

      so my take here is that I think we'd probably be better off retraining folks (./pants test lint) or using a goal alias mechanism (./pants ci) for these types of goals now for a superior UX across the board. ultimately, users on both ends of the spectrum matter and I don't think it's a huge ask to ask folks to learn one extra goal vs actually cutting off an entire camp of power users that would rather compose.

    9. =Kris. test requires compile because you can't test without compiling. But you can compile without linting so there's no reason to force people who want to compile to undergo linting.

    10. It's important to note that putting it at the end of test doesn't force users to undergo anything. You get your test results in the exact amount of time you would have otherwise. If the tests pass AND you want to re-run them immediately without waiting for the linting to finish, you can always ctrl+c, but that seems rare.

    11. Yes, but it still fails the build, which just seems wrong to me. I asked for tests, I expect that to succeed if all the tests pass.

  2. 
      
  1. Some formatting nits along with two bigger questions. Thanks for taking another pass!

  2. pants.ini (Diff revision 1)
     
     
    
      
  3. There is no default configured within the task to fallback to.

    Note that I do not believe you can set this to the one under build-support - consumers of the releases get Pants as an sdist and I do not believe that the build-support files are included.

    1. This is not a pants default, the default is part of the scalafmt binary. If you don't specify a config scalafmt uses its built in default style

    2. Ah, understood. It was not clear to me that falling back the to default did not mean the included config file.

      I don't mind if Pants uses the included file in its pants.ini, but you might as well delete it if you are no longer planning on setting in config.

  4. excess whitespace here

  5. Another pitch to please convert this to check for instances of ScalaLibrary or JavaTests.

    At the minimum, this will result in many excess directory scans, as it checks the extension of every source file of every target in context, disregarding whether it is Python, or Go, or otherwise.

    Imagine someone (it's me) has some horrible legacy tests that sanity-check code generators by running them against checked-in codegen. Operating over every target in context essentially makes this unusable in that case.

    1. I'm not exactly sure how to do this, can you point me at an example? If not I think this is one of the fixes @stuhood said he would take a look at fixing associated with issue #3957

    2. Sure. If stu wants to cover this, that is fine. If you wanted to keep using the lambda, you can change the target checks doing something like:

      --- a/src/python/pants/backend/jvm/tasks/scalafmt.py
      +++ b/src/python/pants/backend/jvm/tasks/scalafmt.py
      @@ -13,7 +13,8 @@ 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
      -
      +from pants.backend.jvm.targets.scala_library import ScalaLibrary
      +from pants.backend.jvm.targets.java_tests import JavaTests
      
       class ScalaFmt(NailgunTask, AbstractClass):
         """Abstract class to run ScalaFmt commands.
      @@ -78,7 +79,7 @@ class ScalaFmt(NailgunTask, AbstractClass):
      
         def get_non_synthetic_scala_targets(self, targets):
           return filter(
      -      lambda target: isinstance(target, Target)
      +      lambda target: isinstance(target, ScalaLibrary) or isinstance(target, JavaTests)
                            and target.has_sources(self._SCALA_SOURCE_EXTENSION)
                            and (not target.is_synthetic),
             targets)
      

      This will now only check and only format ScalaTargets.

    3. I just thought about this - it may be off. Let me actually run this and paste a verified diff.

    4. Ha, okay. It works. I became worried that JavaTests was no longer a thing. Sorry for the email churn!

    5. I'm pretty sure this will not make a noticeable difference in runtime... and since it won't affect behaviour, I'd like to punt on it in order to get this into rc2.

    6. I see you landed it, which is fine. Just to note, the perf impact was a last ditch attempt to sway opinion here - my real issue is that this will fmt files I cannot have formatted.

    7. my real issue is that this will fmt files I cannot have formatted.

      It's not clear how... can you explain? Are they loaded as resources?

      I can loop around to fix this for rc3, but it sounded like you were interested in getting this into rc2, hence the rush to land it.

  6. Looks like a munged docstring:

    "If the files are not formatted correctly are not an error is raised including the command to run to format the files correctly"

  7. I say kill the process_results comment, value is negligible.

  8. You could make the get_non_synthetic_scala_targets a property and return the actual target list here instead of the <targets> placeholder, which could possibly take a user multiple runs to get right.

  9. It looks to me like if no config file is set, then no config file is passed to the tool, end of story.

    1. right if no config file is passed to the tool, Scalafmt has a default config style built in that it uses

  10. It looks like the config file has the possibility of being pulled from two different sets of options?
    What if I pass a configuration to --compile-scalafmt-configuration=<file_option>? When the fmt runs, it will not see a config file and so will run without one.

    I think that the idea here was that the config file option would hang on the base class, but that won't work here because the base class is not installed in register.py. The option instead surfaces on the individual subclasses.

    You could consider making ScalaCheckFormat be the base class instead, and then just have ScalaFormat override whatever it needs to, maybe.

    Personally, I have greatly struggled to hang options on just the task I want to, and no more.
    Possibly there is no great solution at hand. If you decide that is so, a TODO is probably okay with me.

    1. You could merge these with a subsystem, but it might arguably be more confusing to have a third location that the config comes from than it would be to configure it twice.

    2. Yea I agree having it configured in a third disparate place is a bit jarring. Its also super easy to add an integration test like test_scalafmt_format which just uses your built in config to catch when they diverge.

    3. Well, it would be nice to not require end users to add integration tests to check Pants settings. One option could be to raise an exception in the base task when the two settings are not equal.

      I am okay to drop this with a TODO - maybe stu will have an idea if he is doing a pass later.

  11. If you sort out the configuration option, it would be nice to get some coverage for it.

    1. sure added test cases that don't specify config, to show that it still works, and just uses the scalafmt binaries built in config

  12. needs a space after the equals sign.

  13. space after colon in a couple places in this file.

  14. 
      
  1. I left responses to the open issues as comments under the original review. Thanks, Catie!

  2. src/python/pants/backend/jvm/tasks/scalafmt.py (Diff revisions 2 - 3)
     
     

    The config{} doesn't seem right. If you ran this on Pants right now it looks like it would print something like ./pants fmt <targets> configNone`

    Since you marked the option 'advanced' it is fair to assume that it is meant to be set in the pants.ini and just remove the config from the output.

  3. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 0779a823cae955c67948108d2a9e58a1f02db561

Loading...