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

Quick fix for broken used name #449

Merged
merged 1 commit into from Nov 6, 2017

Conversation

Projects
None yet
6 participants
@jilen
Contributor

jilen commented Oct 31, 2017

Fix issue mentioned here sbt/sbt#3666

@eed3si9n eed3si9n added the ready label Oct 31, 2017

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Oct 31, 2017

Member

How feasible is it to create a test representative of the downstream problem?

Member

dwijnand commented Oct 31, 2017

How feasible is it to create a test representative of the downstream problem?

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Oct 31, 2017

Member

@jilen My feedback has not been fully addressed 😉

Member

jvican commented Oct 31, 2017

@jilen My feedback has not been fully addressed 😉

@jilen

This comment has been minimized.

Show comment
Hide comment
@jilen

jilen Oct 31, 2017

Contributor

@jvican
Where should I put the function ? Sorry, I am not quite familiar with sbt.
Is it ok to create an private function called escapseControlChars inside UsedName object ?

Contributor

jilen commented Oct 31, 2017

@jvican
Where should I put the function ? Sorry, I am not quite familiar with sbt.
Is it ok to create an private function called escapseControlChars inside UsedName object ?

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Oct 31, 2017

Member

That's the ideal solution, thanks @jilen 😄

Member

jvican commented Oct 31, 2017

That's the ideal solution, thanks @jilen 😄

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Oct 31, 2017

Member

@dwijnand We have property-based testing in https://github.com/sbt/zinc/blob/1.x/internal/zinc-persist/src/test/scala/sbt/inc/AnalysisGenerators.scala.
Would adding \n in

  def genScalaName: Gen[String] = Gen.listOf(Gen.choose('!', 'Z')).map(_.toString())

repro this issue?

Member

eed3si9n commented Oct 31, 2017

@dwijnand We have property-based testing in https://github.com/sbt/zinc/blob/1.x/internal/zinc-persist/src/test/scala/sbt/inc/AnalysisGenerators.scala.
Would adding \n in

  def genScalaName: Gen[String] = Gen.listOf(Gen.choose('!', 'Z')).map(_.toString())

repro this issue?

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Oct 31, 2017

Member

I don't know but I like the sound of that.

Member

dwijnand commented Oct 31, 2017

I don't know but I like the sound of that.

@jilen

This comment has been minimized.

Show comment
Hide comment
@jilen

jilen Nov 1, 2017

Contributor

@eed3si9n @jvican It is ready for review now.
Actually this bug waste at least 30 minutes (due to recompiling the whole project) per day.
Hope it could be fixed soon both for sbt 1.0 and sbt 0.13.x

Contributor

jilen commented Nov 1, 2017

@eed3si9n @jvican It is ready for review now.
Actually this bug waste at least 30 minutes (due to recompiling the whole project) per day.
Hope it could be fixed soon both for sbt 1.0 and sbt 0.13.x

@jvican

Hey @jilen, closer to the finish line!

Note: this fix will only be available in sbt 1.0. I think you should tell the Quill team that using newlines in names is a bad idea: it makes their code unfriendly to tools and unparseable.

Show outdated Hide outdated internal/zinc-core/src/main/scala/sbt/internal/inc/UsedName.scala Outdated
@@ -170,7 +170,9 @@ trait AnalysisGenerators {
external <- someOf(src.external.all.toList)
} yield Relations.makeClassDependencies(Relation.empty ++ internal, Relation.empty ++ external)
def genScalaName: Gen[String] = Gen.listOf(Gen.choose('!', 'Z')).map(_.toString())
def genScalaName: Gen[String] = {
Gen.listOf(Gen.oneOf(Gen.choose('!', 'Z'), Gen.const('\n'))).map(_.toString())

This comment has been minimized.

@jvican

jvican Nov 1, 2017

Member

Did you try this before and after the change to make sure that it reproduced the bug?

@jvican

jvican Nov 1, 2017

Member

Did you try this before and after the change to make sure that it reproduced the bug?

This comment has been minimized.

@jilen

jilen Nov 2, 2017

Contributor

@jvican Yes

@jilen

jilen Nov 2, 2017

Contributor

@jvican Yes

@typesafe-tools

This comment has been minimized.

Show comment
Hide comment
@typesafe-tools

typesafe-tools Nov 6, 2017

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt 1.x sbt/sbt@9232a8f
zinc pull/449/head 3a6c114
io 1.x sbt/io@e8c9757
librarymanagement 1.x sbt/librarymanagement@91b56c5
util 1.x sbt/util@f4eadfc
website 1.x

The result is: SUCCESS
(restart)

typesafe-tools commented Nov 6, 2017

The validator has checked the following projects against Scala 2.12,
tested using dbuild, projects built on top of each other.

Project Reference Commit
sbt 1.x sbt/sbt@9232a8f
zinc pull/449/head 3a6c114
io 1.x sbt/io@e8c9757
librarymanagement 1.x sbt/librarymanagement@91b56c5
util 1.x sbt/util@f4eadfc
website 1.x

The result is: SUCCESS
(restart)

@jvican

jvican approved these changes Nov 6, 2017

@jvican jvican merged commit cea21b4 into sbt:1.x Nov 6, 2017

1 check passed

continuous-integration/drone/pr the build was successful
Details

@eed3si9n eed3si9n removed the ready label Nov 6, 2017

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 23, 2018

Member

This change represents a slight performance penalty: replaceAllLiterally calls Matcher.quoteReplacement each time on the name.

replace('\n', '\u26680A') would be more efficient.

Member

retronym commented Aug 23, 2018

This change represents a slight performance penalty: replaceAllLiterally calls Matcher.quoteReplacement each time on the name.

replace('\n', '\u26680A') would be more efficient.

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Aug 23, 2018

Member

@jilen mind sending a PR to make that improvement?

Member

jvican commented Aug 23, 2018

@jilen mind sending a PR to make that improvement?

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym
Member

retronym commented Aug 23, 2018

#583

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