Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add-Yprint-trees:diff to make-Vprint show diffs between phases#9987

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
SethTisue merged 1 commit intoscala:2.13.xfromsom-snytt:issues/12548-followup
May 12, 2022

Conversation

som-snytt
Copy link
Contributor

@som-snyttsom-snytt commentedApr 3, 2022•
edited
Loading

When printing compiler ASTs with-Yprint, enable printing just thediff by-Yprint-trees:diff.

That is useful when you don't need to see the whole tree, but you want to see when some feature is introduced into the tree, or observe only the changes at a phase.

Now-Yprint-trees:help reports

Usage: -Yprint-trees:<style> where <style> choices are text, compact, format, text+format, diff (default: text).

What do those words mean?compact isTree DSL style.format is less compact.text means sourcelike.text+format just outputs both styles.diff is just a text-based diff of thetext style.

This PR also upgrades to thedifflib the seems to have usurped the version from 2011.

We ought to embrace our differences. Vive la différence.

SethTisue, ekrich, griggt, and Jasper-M reacted with heart emoji
@scala-jenkinsscala-jenkins added this to the2.13.9 milestoneApr 3, 2022
@som-snyttsom-snytt marked this pull request as draftApril 3, 2022 19:36
@som-snyttsom-snytt marked this pull request as ready for reviewApril 3, 2022 21:20
@lrytz
Copy link
Member

Oh nice! Seems to play well withydiff

scpr 9987 Test.scala -Vprint:_ -Yprint-trees:diff | ydiff

image

Copy link
Member

@dwijnanddwijnand left a comment

Choose a reason for hiding this comment

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

TIL -Yprint-trees exists. LGTM.

Copy link
Member

@SethTisueSethTisue left a comment

Choose a reason for hiding this comment

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

I don't think much of a test suite is needed, but how about including one smoke test, just to make sure the feature doesn't stop working entirely in the future?

build.sbt Outdated
@@ -44,7 +44,8 @@ val jlineDep = "org.jline" % "jline"
val jnaDep = "net.java.dev.jna" % "jna" % versionProps("jna.version")
val jlineDeps = Seq(jlineDep, jnaDep)
val testInterfaceDep = "org.scala-sbt" % "test-interface" % "1.0"
val diffUtilsDep = "com.googlecode.java-diff-utils" % "diffutils" % "1.3.0"
//val diffUtilsDep = "com.googlecode.java-diff-utils" % "diffutils" % "1.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

(remove commented-out line)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

OK, as usual, was afraid of losing visible info.

@@ -23,6 +23,7 @@ libraryDependencies ++= Seq(
"org.eclipse.jgit" % "org.eclipse.jgit" % "4.11.9.201909030838-r",
"org.slf4j" % "slf4j-nop" % "1.7.36",
"com.googlecode.java-diff-utils" % "diffutils" % "1.3.0",
//"io.github.java-diff-utils" % "java-diff-utils" % "4.11",
Copy link
Member

Choose a reason for hiding this comment

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

remove?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

oops, I see the comment was for the new jar, and the question was, Is the other jar needed here for some reason?

Copy link
ContributorAuthor

@som-snyttsom-snyttMay 10, 2022•
edited
Loading

Choose a reason for hiding this comment

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

yes, it's used in build.sbt by the incredibly fragilescala/runtime patch fortest/instrumented which should be renamedtest/demented. I couldn't remove an import at some point in the past because of this mechanism. I'll try to understand what to do now. (That's what I meant above by "the build task". I guess there is no urgency in updating that detail of the build.)

@SethTisue
Copy link
Member

SethTisue commentedApr 20, 2022•
edited
Loading

Is there anywhere this could/should be documented?

At minimum, the PR should have a nice user-facing (meaning, compiler-hacker-facing) PR description, since this is where people'll land if they Google, if there isn't any other documentation.

@SethTisue
Copy link
Member

SethTisue commentedApr 20, 2022•
edited
Loading

Does the underlying library support ANSI color, by any chance? would be cool to have the red/green for old/new

above Lukas shows thatydiff can be used to supply that, but (fwiw) neither Dale and I knewydiff existed

@SethTisueSethTisue added the internalnot resulting in user-visible changes (build changes, tests, internal cleanups) labelApr 20, 2022
@SethTisueSethTisue changed the title-Yprint-trees:diffAdd-Yprint-trees:diff to make-Vprint show diffs between phasesApr 20, 2022
@som-snytt
Copy link
ContributorAuthor

2 weeks ago I started to sayTIL ydiff then got distracted, thanks@lrytz

@lrytz
Copy link
Member

I got to ydiff by wanting to ask about colored output, then googling etc.

@som-snytt
Copy link
ContributorAuthor

When Lukas pulls the curtain away from the man who stands behind it.

lrytz reacted with laugh emoji

@SethTisue
Copy link
Member

@som-snytt mind improving the PR description before we merge?

@som-snytt
Copy link
ContributorAuthor

I will follow up the clean up items.

@som-snyttsom-snyttforce-pushed theissues/12548-followup branch from7fe3dc9 toe792225CompareMay 10, 2022 15:09
@som-snytt
Copy link
ContributorAuthor

I was considering other print features, such as showing when synthetics are added, usingsynths-since.

Then we could say, Lukas has a sixth sense for synths since.

@SethTisueSethTisue added the release-notesworth highlighting in next release notes labelMay 12, 2022
@SethTisueSethTisue merged commita051c92 intoscala:2.13.xMay 12, 2022
@som-snyttsom-snytt deleted the issues/12548-followup branchMay 12, 2022 14:51
@SethTisueSethTisue removed the release-notesworth highlighting in next release notes labelAug 31, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@SethTisueSethTisueSethTisue left review comments

@dwijnanddwijnanddwijnand approved these changes

Assignees
No one assigned
Labels
internalnot resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
Milestone
2.13.9
Development

Successfully merging this pull request may close these issues.

5 participants
@som-snytt@lrytz@SethTisue@dwijnand@scala-jenkins
[8]ページ先頭
©2009-2025 Movatter.jp