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

indentWithTabs removes leading single space #81

Closed
sormuras opened this issue Mar 8, 2017 · 16 comments
Closed

indentWithTabs removes leading single space #81

sormuras opened this issue Mar 8, 2017 · 16 comments

Comments

@sormuras
Copy link
Contributor

sormuras commented Mar 8, 2017

Having standard Javadoc comments, like:

/**
 * <h3>Logging Configuration</h3>
 *
...

After running...

spotless {
	java {
		googleJavaFormat('1.3') // indents with 2 spaces
		indentWithTabs(2)
	}
}

All Javadoc stars are in the fly-on-the-left-wall position:

/**
* <h3>Logging Configuration</h3>
*
...

See https://github.com/junit-team/junit5/compare/google_java_format#diff-65f0a88c2a9307f19525f2d3c03dc8f3 for a complete example.

@sormuras
Copy link
Contributor Author

sormuras commented Mar 8, 2017

@jbduncan
Copy link
Member

jbduncan commented Mar 8, 2017

As a short term measure, how about a custom lazy Spotless task that applies IndentStep to all lines except those that start with *?

@jbduncan
Copy link
Member

jbduncan commented Mar 8, 2017

Of course, this feature addition would be nice to have in the medium term. :)

@nedtwigg
Copy link
Member

nedtwigg commented Mar 8, 2017

A short term fix is

...
    indentWithTabs(2)
    replaceRegex 'Javadoc space indent', '^\\*', ' *'  // this regex might not be exactly right, but it's close

I'm hesitant to change IndentStep. It is a possibly-breaking change in one of the oldest rules we've got. The current implementation can catch bugs of the following sort:

\t    \t    Good
\t    \t    Good
\t    \t    _Oops (where _ = a space)
\t    \t    Good

The suggested change would break this case. Given how easy the regex fix is, I'm inclined to not alter the behavior of IndentStep.

@jbduncan
Copy link
Member

jbduncan commented Mar 8, 2017

The regex \R\* seems to work when I use Ctrl+F in IntelliJ IDEA to find non-indented * in Javadoc comments. So maybe something like the following custom rule would work instead:

indentWithTabs(2)
replaceRegex 'Javadoc space indent', '\R\*', '\n *'

@jbduncan
Copy link
Member

jbduncan commented Mar 8, 2017

Oh no never mind, looks like JUnit 5's build.gradle actually already has a replaceRegex step that can do this and which you can steal @sormuras. :)

replaceRegex 'class-level Javadoc indentation fix', /^\*/, ' *'

@jbduncan
Copy link
Member

jbduncan commented Mar 8, 2017

I've realised that the regex I mentioned above doesn't actually work for method javadocs (or other javadocs that are intended as a whole). The following steps seem to solve all problems (on one of my personal projects at least):

    googleJavaFormat '1.3'
    indentWithTabs 2
    replaceRegex 'class-level javadoc indentation fix', /^\*/, ' *'
    replaceRegex 'method-level javadoc indentation fix', /\t\*/, '\t *'

@MahatmaFatalError
Copy link

will there be an official fix anytime?

@vpavic
Copy link
Contributor

vpavic commented Dec 29, 2021

@nedtwigg This really feels like something that should work out of the box. Virtually everyone who wants to use indentWithTabs and writes javadocs needs to configure those two replaceRegex statements.

@nedtwigg
Copy link
Member

Proposal 1 (bad idea, probably wouldn't merge a PR)

We have a mechanism for implicitly modifying license steps based on context, e.g. this is how we keep the license step from messing up package-info.java

steps.replaceAll(step -> {
if (isLicenseHeaderStep(step)) {
return step.filterByFile(LicenseHeaderStep.unsupportedJvmFilesFilter());
} else {
return step;
}
});

You could add something like:

if (steps.contains(::isGoogleJavaFormatStep) && steps.contains(::isIdentWithTabsStep)) {
  steps.add(replaceRegex 'class-level javadoc indentation fix', /^\*/, ' *')
  steps.add(replaceRegex 'method-level javadoc indentation fix', /\t\*/, '\t *')
}

I think that's okay, but not great because it's hard for users to opt out of.

Proposal 2 (okay idea, would happily merge a PR)

A better solution would be to support something like this: googleJavaFormat().indentWithTabs(), which I would add roughly here

public GoogleJavaFormatConfig aosp() {
return style("AOSP");
}

and it would look something like this

public void indentWithTabs() {
  // needs to be terminal (returns void) because unlike the other methods in GoogleJavaFormatConfig, this is hard to undo. */
  indentWithTabs(2)
  replaceRegex 'class-level javadoc indentation fix', /^\*/, ' *'
  replaceRegex 'method-level javadoc indentation fix', /\t\*/, '\t *'
}

@vpavic
Copy link
Contributor

vpavic commented Dec 30, 2021

Thanks for following up on this.

From the snippets you shared it appears to me that you're somehow conflating this issue with googleJavaFormat which shouldn't be the case as the problem is much more general.

Here's a minimal reproducer that only contains a build.gradle script:

plugins {
	id("com.diffplug.spotless").version("6.1.0")
}

/**
 * A simple greeting task.
 */
abstract class GreetingTask extends DefaultTask {
	@TaskAction
	def greet() {
		println("hello from GreetingTask")
	}
}

tasks.register("hello", GreetingTask)

spotless {
	groovyGradle {
		indentWithTabs()
	}
}
$ ./gradlew spotlessCheck
> Task :spotlessGroovyGradleCheck FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spotlessGroovyGradleCheck'.
> The following files had format violations:
      build.gradle
          @@ -3,8 +3,8 @@
           }
           
           /***·A·simple·greeting·task.
          -·*/
          +*·A·simple·greeting·task.
          +*/
           abstract·class·GreetingTask·extends·DefaultTask·{
           \t@TaskAction
           \tdef·greet()·{
  Run './gradlew :spotlessApply' to fix these violations.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 888ms
3 actionable tasks: 3 executed

Also, could you reopen the issue and label it as bug instead of question?

@jbduncan
Copy link
Member

jbduncan commented Dec 30, 2021

@vpavic, I could be wrong, but I interpreted @nedtwigg's Proposal 2 as saying, "all Java/Groovy/Kotlin formatters, including 'google-java-format', which I'm just using as an example here". Or am I barking up the wrong tree, Ned? :)

@vpavic
Copy link
Contributor

vpavic commented Dec 30, 2021

I see, thanks.

Hopefully that's the case - admittedly, I haven't been too much into the Spotless internals.

@jbduncan
Copy link
Member

jbduncan commented Dec 30, 2021

@vpavic I hope so too. I don't imagine it would be that hard to make a new indentWithTabs() method for all the Java/Groovy/Kotlin formatters that use a fixed number of spaces for indentation, like how google-java-format predictably uses either 2 spaces or 4 spaces depending on which mode it's using. But I'm not sure about more flexible formatters like Eclipse.

I'm also assuming that implementing an indentWithTabs method like this would work, and that numberOfSpacesUsedForIndentation is easy enough to calculate:

public void indentWithTabs() {
  int numberOfSpacesUsedForIndentation = ...
  indentWithTabs(numberOfSpacesUsedForIndentation)
  replaceRegex 'class-level javadoc indentation fix', /^\*/, ' *'
  replaceRegex 'method-level javadoc indentation fix', /\t\*/, '\t *'
}

@nedtwigg nedtwigg changed the title indentWithTabs removes leading single space indentWithTabs and C-style multiline comments Dec 31, 2021
@nedtwigg
Copy link
Member

nedtwigg commented Jan 6, 2022

conflating this issue with googleJavaFormat ... the problem is much more general.

Most of the other formatters (or at least eclipse and greclipse) allow you to configure indentation to use tabs in the first place, so I did see this as a GJF-specific usecase.

But you raise a good point, C-style multiline comments are not unusual, we should "just work" for that case. I'm going to open another issue for that.

@nedtwigg
Copy link
Member

nedtwigg commented Jan 6, 2022

I don't plan to implement #1070, but happy to merge a PR that does.

@nedtwigg nedtwigg changed the title indentWithTabs and C-style multiline comments indentWithTabs removes leading single space Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants