New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add google-java-format fmt/lint support #5596

Merged
merged 11 commits into from Mar 15, 2018

Conversation

Projects
None yet
2 participants
@traviscrawford
Copy link
Member

traviscrawford commented Mar 14, 2018

To ensure a consistent Java coding style, some users may wish to incorporate google-java-format into their lint/fmt goals. Here we add google-java-cormat support to contrib.

@@ -19,11 +19,13 @@
class ScalaRewriteBase(NailgunTask, AbstractClass):
"""Abstract base class for both scalafix and scalafmt: tools that check/rewrite scala sources."""

_TARGET_TYPES = ['scala_library', 'junit_tests', 'java_tests']

This comment has been minimized.

@traviscrawford

traviscrawford Mar 14, 2018

Member

Here we make the minimally invasive change. What do you think about renaming this file rewrite_base.py and removing the Scala references? This class seems generally relevant to any JVM-based formatter/linter tool, rather than being tied to the language of sources being formatted/linted.

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

I like it. I'd prefer a manually abstract classmethod though that doc could be hung off of, ie:

@classmethod
def default_target_types(cls):
  """Subclasses should ..."""
  raise NotImplementedError()

That said, this is the existing style, so take it or leave it. I just went down the rabbit hole of checking _SCALA_SOURCE_EXTENSION - another example of this - and I find it mystifying as-is since its not defined in the baseclass, not documented, but required to be defined by leaf subclasses in order to not blow up.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 14, 2018

Note: assuming there will be a change to scala_rewrite_base.py, I did not yet add tests, nor register the contrib plugin to be published. I'll get this ready to actually review/merge after getting some guidance on the base class change.

traviscrawford added some commits Mar 14, 2018

@jsirois
Copy link
Member

jsirois left a comment

Thanks Travis! LGTM, but a few ideas to consider.

@@ -19,11 +19,13 @@
class ScalaRewriteBase(NailgunTask, AbstractClass):
"""Abstract base class for both scalafix and scalafmt: tools that check/rewrite scala sources."""

_TARGET_TYPES = ['scala_library', 'junit_tests', 'java_tests']

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

I like it. I'd prefer a manually abstract classmethod though that doc could be hung off of, ie:

@classmethod
def default_target_types(cls):
  """Subclasses should ..."""
  raise NotImplementedError()

That said, this is the existing style, so take it or leave it. I just went down the rabbit hole of checking _SCALA_SOURCE_EXTENSION - another example of this - and I find it mystifying as-is since its not defined in the baseclass, not documented, but required to be defined by leaf subclasses in order to not blow up.


class GoogleJavaFormatBase(ScalaRewriteBase):

_MAIN = 'com.google.googlejavaformat.java.Main'

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

How about inlining this? I think its only needed here and in one place and not a magic _OVERRIDE_ME like the constants below.


@abstractproperty
def additional_args(self):
return []

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

A bit contradictory to be @abstractproperty and have an implementation. How about hanging doc off the method instead? Alternatively, just pass.

TEST_DIR = 'contrib/googlejavaformat/testprojects/src/java/org/pantsbuild/testproject'


class GoogleJavaFormatIntegrationTests(PantsRunIntegrationTest):

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

Since ScalaRewriteBase tasks are self contained and require no product setup it would be a good deal nicer to model this as a pants_test.tasks.task_test_base.TaskTestBase unit test instead where make_target and create_file were used to emit ephemeral source code to lint and re-format. Faster tests plus no muddying checked in files. What do you think? Could be a follow up conversion too.

This comment has been minimized.

@traviscrawford

traviscrawford Mar 14, 2018

Member

Sounds good - I was just cribing from test_scalafmt.py :) Will make this change.

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

Fixed that one!: #5599
Down from ~33s wall time in py.test to ~10s plus now cacheable.

jsirois added a commit to jsirois/pants that referenced this pull request Mar 14, 2018

Convert scalafmt test to a unit test.
This improves local-readability and knocks down the run-time by 2/3.

Inspired by pantsbuild#5596.
@jsirois
Copy link
Member

jsirois left a comment

Thanks for migrating the test @traviscrawford!

@@ -1,8 +1,6 @@
python_tests(
dependencies = [
'contrib/googlejavaformat/src/python/pants/contrib/googlejavaformat',
'tests/python/pants_test:int-test',
'tests/python/pants_test/jvm:nailgun_task_test_base',

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

This could use a few more deps!

self.assertEqual(actual, self._GOODFORMAT)



This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

Kill extra blank line.

)

context = self.context(target_roots=[target])
task = self.prepare_execute(context)

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

You could just self.execute(context) since you don't need the task object returned by prepare_execute. Same goes in a few places below.

This comment has been minimized.

@traviscrawford

traviscrawford Mar 14, 2018

Member

This took some playing around with earlier to get working - without prepare_execute I was running into issues about product's needing to be registered or something like that. I'll take another look at this.

This comment has been minimized.

@traviscrawford

traviscrawford Mar 14, 2018

Member

Must have been related to something else - fixed! Now that I think about it, I am probably mixing this up with an error from before I switched to subclassing NailgunTaskTestBase.

return GoogleJavaFormat

@property
def alias_groups(self):

This comment has been minimized.

@jsirois

jsirois Mar 14, 2018

Member

Looks like this could lift to BaseTest since both subclasses use it.

Travis Crawford added some commits Mar 14, 2018

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 14, 2018

Thanks for the feedback - I believe I addressed all your suggestions, and merged master. LMK if there's anything else you think would make this better.

@traviscrawford

This comment has been minimized.

Copy link
Member

traviscrawford commented Mar 15, 2018

Oof, one more change. I ran ./pants fmt before-hand, but didn't realize it's not configured for Python. I verified ./pants lint passes.

@jsirois jsirois merged commit 62a0696 into pantsbuild:master Mar 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Mar 15, 2018

Thanks again @traviscrawford, this is now in master.

jsirois added a commit that referenced this pull request Mar 15, 2018

Convert scalafmt test to a unit test. (#5599)
This improves local-readability and knocks down the run-time by 2/3.

Inspired by #5596.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment