Skip to content

indentWithSpaces doesn't check the number of spaces used #117

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

Closed
boris-petrov opened this issue May 30, 2017 · 9 comments
Closed

indentWithSpaces doesn't check the number of spaces used #117

boris-petrov opened this issue May 30, 2017 · 9 comments
Labels

Comments

@boris-petrov
Copy link

boris-petrov commented May 30, 2017

I have something like:

spotless {
    format 'xml', {
        target 'src/**/*.xml'
 
        trimTrailingWhitespace()
        indentWithSpaces()
        endWithNewline()
    }
}

And an XML file. indentWithSpaces works in the sense that it checks for spaces - i.e. if I have tabs in an XML file, it will catch that. However, it doesn't catch if I indent with a wrong amount of spaces - e.g. 2 - even if I put an explicit count (indentWithSpaces(4)). I am missing something or is this a known limitation? Or perhaps a bug?

P.S. I guess this is related to #58. I guess this won't be "fixed"?

@jbduncan
Copy link
Member

This is known and intentional limitation, unfortunately. See #58 for the rationale behind this decision. :)

@boris-petrov
Copy link
Author

As far as I understand the problem comes from block comments? Actually because of them I have commented out indentWithTabs for my java formatting - as I use tabs for Java but have block comments which cause errors (because of the same reason - a single space before the *).

So I guess block comments should be treated as a special case for both indentation count and indentation type (spaces/tabs). I haven't read your code and have no idea what you have implemented or use as third-party helpers so I cannot give any helpful ideas on how to do that. You're saying that it is difficult/impossible? Or am I misunderstanding something?

@jbduncan
Copy link
Member

jbduncan commented May 30, 2017

For us, it was a question of difficulty and weighing up whether trying to solve the problem was going to be worth the effort. We came to the conclusion at the time that it wasn't worth the effort, as we couldn't think of a useful compromise between the current behaviour of intendWithTabs and making it function like what you're asking for without accidentally mangling JavaDoc comments and the like, and it wasn't something @nedtwigg or I desperately needed.

I'm actually not entirely sure why @nedtwigg closed #58, but I'd be happy to re-open it if you or anyone else has any suggestions. :)

@jbduncan
Copy link
Member

Heck, we're very open to PRs, which we'd be happy to guide people with, like describing which part of the Spotless codebase does what, and which part(s) probably matter for the given problem (although @nedtwigg may be better at this than me, as my knowledge of the codebase is still spotty). :)

@jbduncan
Copy link
Member

I hope this helps?

@boris-petrov
Copy link
Author

Well it does help in the sense that I see you want this fixed, just haven't managed to get to it. :) I suggest you leave the issue open so people would not be confused like me and maybe someone someday will come up with a pull-request. :)

@nedtwigg
Copy link
Member

I guess this won't be "fixed"?

Correct! As the linked issue describes, you really need a format-specific parser which understands the text better than a simple regex.

Keeping issue #58 open

Even though it's closed, anybody can comment on it, and if there's a promising path forward I'm happy to reopen it. As the issue describes, I don't think there is a path forward.

An issue tracker is only as good as the work it organizes. IMO, that means that an open issue means "We are taking action on this" or "We will take action on this". Once it is clear that forward progress is unlikely, I close it. Doesn't mean it can't be discussed further, but it helps show that our open issues really mean something.

@phillipuniverse
Copy link

Ran across this myself and figured I would comment on this and let everyone know that a solution exists!

With the changes at #315 to work with WTP, we can now do proper XML formatting. Here's what I've got (more general instructions in the README:

xml.prefs:

eclipse.preferences.version=1
formatCommentJoinLines=false
indentationChar=space
indentationSize=4
lineWidth=100

and the Gradle step:

format 'xml', {
    target fileTree('.') {
      include '**/*.xml'
      exclude '**/build/**'
    }
    eclipseWtp('xml').configFile 'xml.prefs'
  }

I verified that this now correctly validates that my XML files have 4-space indents and can only be 100 lines long.

@nedtwigg
Copy link
Member

Thanks for the update @phillipuniverse, and thanks to @fvgh for all the hard work making the wtp formatter work!

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

No branches or pull requests

4 participants