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

Space removed after annotation - Want to keep it #618

Closed
d1egoaz opened this Issue Dec 19, 2016 · 19 comments

Comments

3 participants
@d1egoaz

d1egoaz commented Dec 19, 2016

When using Play DI Guice, I would like to keep the space after the @Inject annotation

Version: scalafmt 0.5.0-RC2
Integration: cli
Configuration: style=defaultWithAlign

class Foo @Inject() (conf: Configuration)

Scalafmt:

class Foo @Inject()(conf: Configuration)

I would like the input to look like this:

class Foo @Inject() (conf: Configuration)

I think I can help with a PR if you could point me in the right direction/where to start.

And thank you for this amazing project 🍻 , I've been using it for a few weeks now, it has been really useful since I moved from IntelliJ to Emacs

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Dec 19, 2016

Member

Thanks for reporting! If you are motivated to take a stab at this, you can track down where the NoSplit (no space) origins between )( using the tutorial: https://github.com/olafurpg/scalafmt/blob/master/Tutorial.md

Each whitespace token (NoSplit, Space or Newline) between non-whitespace tokens can be tracked down to a line number in Router.scala.

Member

olafurpg commented Dec 19, 2016

Thanks for reporting! If you are motivated to take a stab at this, you can track down where the NoSplit (no space) origins between )( using the tutorial: https://github.com/olafurpg/scalafmt/blob/master/Tutorial.md

Each whitespace token (NoSplit, Space or Newline) between non-whitespace tokens can be tracked down to a line number in Router.scala.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 2, 2017

@olafurpg I ran "sbt test"
I get this error (screenshot attached). Tried googling but no success.
screenshot from 2017-03-02 22-46-17

AnamikaD commented Mar 2, 2017

@olafurpg I ran "sbt test"
I get this error (screenshot attached). Tried googling but no success.
screenshot from 2017-03-02 22-46-17

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 3, 2017

Member

@AnamikaD Great you got the tests running! That intellij/compile failure is not your fault, it's a bug in the build that I fix in https://github.com/olafurpg/scalafmt/pull/760 To get intellij/compile to work, you need to run intellij/updateIdea first, and that will download a lot of files (~800mb) that are only relevant when developing the intellij plugin.

Member

olafurpg commented Mar 3, 2017

@AnamikaD Great you got the tests running! That intellij/compile failure is not your fault, it's a bug in the build that I fix in https://github.com/olafurpg/scalafmt/pull/760 To get intellij/compile to work, you need to run intellij/updateIdea first, and that will download a lot of files (~800mb) that are only relevant when developing the intellij plugin.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 3, 2017

@olafurpg Thank you for the reply! I will go forward with the code and other tasks given.

AnamikaD commented Mar 3, 2017

@olafurpg Thank you for the reply! I will go forward with the code and other tasks given.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 5, 2017

@olafurpg According to this: https://github.com/olafurpg/scalafmt/blob/master/Tutorial.md, In Router.scala line 1276 contains " Split(NoSplit, 0) ", but I see this: " Split(Space, 0, ignoreIf = newlines > 0) "!

AnamikaD commented Mar 5, 2017

@olafurpg According to this: https://github.com/olafurpg/scalafmt/blob/master/Tutorial.md, In Router.scala line 1276 contains " Split(NoSplit, 0) ", but I see this: " Split(Space, 0, ignoreIf = newlines > 0) "!

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 5, 2017

Member

@AnamikaD Yes, that can very likely be. The tutorial was last updated in early January and Router.scala has changed a bit since then. If you run the tests yourself, you should be able to see the new line numbers.

Member

olafurpg commented Mar 5, 2017

@AnamikaD Yes, that can very likely be. The tutorial was last updated in early January and Router.scala has changed a bit since then. If you run the tests yourself, you should be able to see the new line numbers.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 5, 2017

@olafurpg Thanks! Got it.

AnamikaD commented Mar 5, 2017

@olafurpg Thanks! Got it.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 5, 2017

@olafurpg
In Hacking.stat:
maxColumn = 100
<<< ONLY Spaces in after close braces
class Foo @Inject()(conf: Configuration)

class Foo @Inject() (conf: Configuration)

In Router.scala, line 356 added this,
if (style.spaces.afterCloseBraces) Space
else NoSplit
afterCloseBraces is defined true in Space.scala. Test case failed. Any leads/Guide??

AnamikaD commented Mar 5, 2017

@olafurpg
In Hacking.stat:
maxColumn = 100
<<< ONLY Spaces in after close braces
class Foo @Inject()(conf: Configuration)

class Foo @Inject() (conf: Configuration)

In Router.scala, line 356 added this,
if (style.spaces.afterCloseBraces) Space
else NoSplit
afterCloseBraces is defined true in Space.scala. Test case failed. Any leads/Guide??

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 5, 2017

Member

@AnamikaD Please try to wrap code block with triple backticks `, that makes it easier to read.

Have you tried to set

spaces.afterCloseBraces = true

after the maxColumns line?

BTW. I don't think there's a need to introduce a new flag for this change, we can include the change in the default style.

Member

olafurpg commented Mar 5, 2017

@AnamikaD Please try to wrap code block with triple backticks `, that makes it easier to read.

Have you tried to set

spaces.afterCloseBraces = true

after the maxColumns line?

BTW. I don't think there's a need to introduce a new flag for this change, we can include the change in the default style.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 6, 2017

@olafurpg sorry for the trouble.
Failed test case. Where is this default style? do we need to check whether there is an '(' after ')'?
Hacking.stat code:

maxColumn = 100
spaces.afterCloseBraces = true
<<< ONLY Spaces in after close braces
class Foo @Inject()(conf: Configuration)
>>>
class Foo @Inject() (conf: Configuration) 

AnamikaD commented Mar 6, 2017

@olafurpg sorry for the trouble.
Failed test case. Where is this default style? do we need to check whether there is an '(' after ')'?
Hacking.stat code:

maxColumn = 100
spaces.afterCloseBraces = true
<<< ONLY Spaces in after close braces
class Foo @Inject()(conf: Configuration)
>>>
class Foo @Inject() (conf: Configuration) 
@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 6, 2017

Member

The default style is the configuration that is used everywhere unless you override settings with for example maxColumn = 100.

That test looks good, except you can remove the spaces.afterCloseBraces = true part. I think we want to handle the case here so that the split is a Space when leftOwner.parent.exists(_.is[Mod])

Member

olafurpg commented Mar 6, 2017

The default style is the configuration that is used everywhere unless you override settings with for example maxColumn = 100.

That test looks good, except you can remove the spaces.afterCloseBraces = true part. I think we want to handle the case here so that the split is a Space when leftOwner.parent.exists(_.is[Mod])

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 6, 2017

@olafurpg what is leftOwner and Mod? what does it mean?

AnamikaD commented Mar 6, 2017

@olafurpg what is leftOwner and Mod? what does it mean?

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 6, 2017

The new line after '(' is caused by,
here
I get the output as

class Foo @Inject() (
  conf: Configuration )

AnamikaD commented Mar 6, 2017

The new line after '(' is caused by,
here
I get the output as

class Foo @Inject() (
  conf: Configuration )

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 7, 2017

Member

@AnamikaD

what is leftOwner and Mod? what does it mean?

leftOwner is the "owner" or "parent" of the left token. Scalafmt views code as an abstract syntax tree where the leafs of the tree are tokens, something like this

When formatting something like class Foo @Inject() (x: Int), scalafmt has the task of deciding what to but between non-whitespace token, something like filling the _ parts with space/newline/empty characters

     _   _ _      _ _ _
class Foo @ Inject ( ) (

Mod is a modifier tree node in the abstract syntax tree, for example like @Inject(). To learn more about syntax trees and tokens in scala, you might want to go through http://scalameta.org/tutorial/ I can highly recommend installing the Ammonite repl and play around with how the syntax tree of different code looks like,

$ amm // start ammonite repl from terminal console
Loading...
Welcome to the Ammonite Repl 0.8.2
(Scala 2.12.1 Java 1.8.0_102)
@ import $ivy.`org.scalameta:contrib_2.12:1.6.0`, scala.meta._, contrib._ // this loads the scala.meta dependency
import $ivy.$                                 , scala.meta._, contrib._
@ q"class Foo @Inject() (x: Int)".structure // this command prints out the abstract syntax tree of that code
res1: String = """
Defn.Class(Nil, Type.Name("Foo"), Nil, Ctor.Primary(Seq(Mod.Annot(Term.Apply(Ctor.Ref.Name("Inject"), Nil))), Ctor.Ref.Name("this"), Seq(Seq(Term.Param(Nil, Term.Name("x"), Some(Type.Name("Int")), None)))), Template(Nil, Nil, Term.Param(Nil, Name.Anonymous(), None, None), None))
"""  // Notice the `Mod.Annot` part
Member

olafurpg commented Mar 7, 2017

@AnamikaD

what is leftOwner and Mod? what does it mean?

leftOwner is the "owner" or "parent" of the left token. Scalafmt views code as an abstract syntax tree where the leafs of the tree are tokens, something like this

When formatting something like class Foo @Inject() (x: Int), scalafmt has the task of deciding what to but between non-whitespace token, something like filling the _ parts with space/newline/empty characters

     _   _ _      _ _ _
class Foo @ Inject ( ) (

Mod is a modifier tree node in the abstract syntax tree, for example like @Inject(). To learn more about syntax trees and tokens in scala, you might want to go through http://scalameta.org/tutorial/ I can highly recommend installing the Ammonite repl and play around with how the syntax tree of different code looks like,

$ amm // start ammonite repl from terminal console
Loading...
Welcome to the Ammonite Repl 0.8.2
(Scala 2.12.1 Java 1.8.0_102)
@ import $ivy.`org.scalameta:contrib_2.12:1.6.0`, scala.meta._, contrib._ // this loads the scala.meta dependency
import $ivy.$                                 , scala.meta._, contrib._
@ q"class Foo @Inject() (x: Int)".structure // this command prints out the abstract syntax tree of that code
res1: String = """
Defn.Class(Nil, Type.Name("Foo"), Nil, Ctor.Primary(Seq(Mod.Annot(Term.Apply(Ctor.Ref.Name("Inject"), Nil))), Ctor.Ref.Name("this"), Seq(Seq(Term.Param(Nil, Term.Name("x"), Some(Type.Name("Int")), None)))), Template(Nil, Nil, Term.Param(Nil, Name.Anonymous(), None, None), None))
"""  // Notice the `Mod.Annot` part
@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 8, 2017

@olafurpg Good explanation! I will walk through the tutorials asap. Can you tell me why are we getting a new line after '@Inject() (' ? (my previous comment).

AnamikaD commented Mar 8, 2017

@olafurpg Good explanation! I will walk through the tutorials asap. Can you tell me why are we getting a new line after '@Inject() (' ? (my previous comment).

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Mar 8, 2017

Member

@AnamikaD The tutorial explain how to trace a newline/space down to a line number in Router.scala, maybe the maxColumn is set too low?

Member

olafurpg commented Mar 8, 2017

@AnamikaD The tutorial explain how to trace a newline/space down to a line number in Router.scala, maybe the maxColumn is set too low?

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 8, 2017

@olafurpg previously, went through the line number, made few changes still the output was incorrect. So after increasing maxColumn, it worked though I have 5 test cases failed. Attached screen shot
screenshot from 2017-03-08 19-18-01
.

AnamikaD commented Mar 8, 2017

@olafurpg previously, went through the line number, made few changes still the output was incorrect. So after increasing maxColumn, it worked though I have 5 test cases failed. Attached screen shot
screenshot from 2017-03-08 19-18-01
.

@AnamikaD

This comment has been minimized.

Show comment
Hide comment
@AnamikaD

AnamikaD Mar 14, 2017

#Fix #618, add space after annotation #835

AnamikaD commented Mar 14, 2017

#Fix #618, add space after annotation #835

AnamikaD added a commit to AnamikaD/scalafmt that referenced this issue Mar 18, 2017

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg May 14, 2018

Member

Closing due to inactivity, please reopen if you would like to take a stab at this!

Member

olafurpg commented May 14, 2018

Closing due to inactivity, please reopen if you would like to take a stab at this!

@olafurpg olafurpg closed this May 14, 2018

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