Skip to content
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

Improve gradle-baseline-java integration with IntelliJ import #1411

Merged
merged 12 commits into from
Jun 17, 2020

Conversation

aldexis
Copy link
Contributor

@aldexis aldexis commented Jun 12, 2020

Before this PR

When directly importing the gradle project in IntelliJ, neither checkstyle nor copyright would be set up properly

After this PR

==COMMIT_MSG==
Adds the proper configuration files upon IntelliJ import of a gradle project for checkstyle and copyright.

This generates the following additional files:

  • .idea/copyright/profiles_settings.xml
  • an xml file under .idea/copyright/ per copyright file under .baseline/copyright
  • .idea/checkstyle-idea.xml (and adds Checkstyle-IDEA to the external dependencies) if baseline-checkstyle is applied
  • Either .idea/codeStyleSettings.xml or a .idea/codeStyles/ folder with the contents being copied from .baseline/idea
    • If .baseline/idea/codeStyles is present, it will copy its contents, otherwise, it will fall back to .baseline/idea/intellij-java-palantir-style.xml as currently
    • The fallback is using a legacy IntelliJ format and requires closing and reopening the project to be taken into account

==COMMIT_MSG==

Possible downsides?

  • Small changes to the existing code structure for non-import code paths, which shouldn't change the output
  • Not impossible this would override or conflict with other setups people have manually done

@changelog-app
Copy link

changelog-app bot commented Jun 12, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Adds the proper configuration files upon IntelliJ import of a gradle project for checkstyle and copyright.

This generates the following additional files:

  • .idea/copyright/profiles_settings.xml
  • an xml file under .idea/copyright/ per copyright file under .baseline/copyright
  • .idea/checkstyle-idea.xml (and adds Checkstyle-IDEA to the external dependencies) if baseline-checkstyle is applied
  • Either .idea/codeStyleSettings.xml or a .idea/codeStyles/ folder with the contents being copied from .baseline/idea
    • If .baseline/idea/codeStyles is present, it will copy its contents, otherwise, it will fall back to .baseline/idea/intellij-java-palantir-style.xml as currently
    • The fallback is using a legacy IntelliJ format and requires closing and reopening the project to be taken into account

Check the box to generate changelog(s)

  • Generate changelog entry

Comment on lines 303 to 307
def checkstyle = project.plugins.findPlugin(BaselineCheckstyle)
if (checkstyle == null) {
project.logger.debug "Baseline: Skipping IDEA checkstyle configuration since baseline-checkstyle not applied"
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd flip this so it performs the action when (and if) the BaselineCheckstyle plugin is applied.
In the current state, you're dependent on the order of application, so if BaselineCheckstyle is applied later, intellij won't be configured correctly.

project.plugins.withType(BaselineCheckstyle) {
  // rest of the function
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied this from the existing checkstyle application. Would you recommend I flip it for both then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only for code paths that happen at configuration time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up flipping it for both addCheckstyle (called from within ideaRootModel.project.ipr.withXml so maybe that wasn't needed?) and addCheckstyleIntellijImport (mostly because I didn't see your edit before pushing, but I also think consistency is better - I'd even want to not have duplicate code but didn't find a nice way to do it because I'm not familiar enough with the code and how gradle configuration works yet)

@dansanduleac
Copy link
Contributor

Played around with this locally while importing with intellij and fixed some issues where we were adding extra nodes (that intellij didn't know how to deal with) instead of modifying existing nodes when they exist.

Specifically we were always adding

  • a new node to .idea/codeStyles/codeStyleConfig.xml's <component>, and
  • a new <code_scheme> to .idea/codeStyles/Project.xml's <component>

even if such node already existed. This is now fixed

@aldexis
Copy link
Contributor Author

aldexis commented Jun 17, 2020

Thanks for fixing @dansanduleac! Seems like we need another person's approval as well since you contributed?

@bulldozer-bot bulldozer-bot bot merged commit 3bd2a1e into develop Jun 17, 2020
@bulldozer-bot bulldozer-bot bot deleted the import-idea branch June 17, 2020 16:36
@svc-autorelease
Copy link
Collaborator

Released 3.28.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants