-
Notifications
You must be signed in to change notification settings - Fork 184
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
Add --diff to report only linter messages introduced by a git diff #472
Conversation
a69b514
to
c894738
Compare
2bb15e3
to
e006831
Compare
This implement the --diff command to use scalafix in an existing codebase. It run the git diff algoritm via JGit and only lint messages where the code is modified. > I figured out the JGit api calls by looking at DiffCommand.java https://github.com/eclipse/jgit/blob/v4.9.1.201712030800-r/org.eclipse.jgit/src/org/eclipse/jgit/api/DiffCommand.java#L113 > to print the git diff from jgit import org.eclipse.jgit.api.Git val git = new Git(repository) git .diff() .setOldTree(oldTree2) .setNewTree(newTree2) .setContextLines(0) .setOutputStream(System.out) .call() > DiffFormatter.setProgressMonitor Will only show when the rename algorithm is used. Not really useful.
e006831
to
f4e0e3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jgit looks great, nice call! Only important comment is the git tests, i wonder if we can reuse the same infrastructure as the other cli tests with only diffs added to the mix.
iterator(repository, _.parseCommit(repository.exactRef(ref).getObjectId())) | ||
|
||
// could be handy in future | ||
private def commit( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is unused then we should remove it.
|
||
def edits(file: FileHeader): Unit = { | ||
val changes = List.newBuilder[GitChange] | ||
file.toEditList.asScala.foreach { edit => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be turned into a map? Builders are great when they're needed but they should be avoided if they can be replaced with a single map (optionally to iterator before and to list after)
diffs += ModifiedFile(path(file.getNewPath), changes.result()) | ||
} | ||
|
||
getDiff(repository, oldTree1, newTree1).foreach { file => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, could this be a flatmap?
| ^ | ||
|""".stripMargin | ||
|
||
println("-------") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No println in tests :)
import scalafix.testkit.DiffAssertions | ||
|
||
class CliGitDiffTests() extends FunSuite with DiffAssertions { | ||
gitTest("it should handle addition") { (fs, git, cli) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name this test "addition"? The convention in scalafix tests is to try keep test names as concise as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with utest it's Possible to run a single test by name, which makes sentences annoying to work with
assertNoDiff(obtained, expected) | ||
} | ||
|
||
gitTest("it should handle modification") { (fs, git, cli) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
} | ||
|
||
private class Fs() { | ||
val workingDirectory: Path = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace this utility with jgit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented too early you are using jgit! 👍
private class Git(workingDirectory: Path) { | ||
import org.eclipse.jgit.api.{Git => JGit} | ||
|
||
private val git = JGit.init().setDirectory(workingDirectory.toFile).call() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to apply a unified diff string? I wonder if we can refactor the tests into a "check" method that takes a name, a StringFS string and expected linter output? Similar to how BaseCliTest supports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, I don't think there is a good opportunity for refactoring. If we add a lot of stuff in BaseCliTest it's going to be unnecessarily complex.
assertNoDiff(obtained, expected) | ||
} | ||
|
||
gitTest("it should handle rename") { (fs, git, cli) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test for unstaged/staged files? From my experience in scalafmt, we should treat those as new files. It's convenient to be able to run scalafix on changes before commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git diff will only pick staged files with git diff --cached
is this what you want?
Ready for round 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the cli now results in new log entries from SLF4J
$ scalafix-nightly -r DisableSyntax --config-str="DisableSyntax.keywords=[def]" --diff
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Can we avoid that entry? I'm fine with adding a dependency on logback.
It seems we are not using --diff to decide which files to run on.
$ scalafix-nightly -r DisableSyntax --config-str="DisableSyntax.keywords=[def]" --diff-branch drop-in-jgit
Running DisableSyntax
100.0% [##########] 249 source files fixed
That diff is empty so I expected it to run scalafix on 0 files instead of 249 files. We use DiffDisable here to filter our mismatching files from result
} else Ok(result) |
It seems a NPE is thrown when we pass jgit a tag instead of a branch
scalafix-nightly -r DisableSyntax --config-str="DisableSyntax.keywords=[def]" --diff-branch=v0.5.6
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
error: java.lang.NullPointerException
scalafix.internal.jgit.JGitDiff$.$anonfun$ref$1(JGitDiff.scala:57)
scalafix.internal.jgit.JGitDiff$.iterator(JGitDiff.scala:63)
scalafix.internal.jgit.JGitDiff$.ref(JGitDiff.scala:57)
scalafix.internal.jgit.JGitDiff$.apply(JGitDiff.scala:32)
scalafix.internal.jgit.DiffDisable$.apply(DiffDisable.scala:17)
All comments addressed, ready for round 3. |
val diffDisable: Option[DiffDisable] = { | ||
if (cli.diff || cli.diffBranch.nonEmpty) { | ||
val baseBranch = cli.diffBranch.getOrElse("master") | ||
Some(DiffDisable(common.workingPath.toNIO, baseBranch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when DiffDisable
fails? For example a non-existent branch is used. We can convert diffDisable
into a Configured[Option[DiffDisable]]
and (resolvedPathMatcher |@| diffDisable).map
import scala.collection.JavaConverters._ | ||
|
||
object JGitDiff { | ||
def apply(workingDir: Path, baseBranch: String): List[GitDiff] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this return Configured[List[GitDiff]]
and use Configured.error("message")
instead of throwing exceptions?
val git = new Git(repository) | ||
val branches = git.branchList().call() | ||
if (branches.isEmpty) { | ||
throw new Exception(s"$workingDir is not a git repository") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use exceptions we should try to create custom exception types instead of java.lang.Exception so that these can be caught without string fiddling.
} | ||
val baseBranchExists = branches.asScala.exists(_.getName() == baseBranchRef) | ||
if (!baseBranchExists) { | ||
throw new Exception(s"Cannot find git branch $baseBranch") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is reported for tags, can we treat tags like branches?
scalafix-nightly -r DisableSyntax --config-str="DisableSyntax.keywords=[def]" --diff-branch=v0.5.6 8 drop-in-jgit
error: java.lang.Exception: Cannot find git branch v0.5.6
scalafix.internal.jgit.JGitDiff$.apply(JGitDiff.scala:40)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rename --diff-branch= to --diff-base= and allow more options (hash and tags)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems some branches are not picked up
ollie ~ dev scalafix scalafix-nightly -r DisableSyntax --config-str="DisableSyntax.keywords=[def]" --diff-branch=vararg-pattern
error: Cannot find git branch vararg-pattern
ollie ~ dev scalafix git checkout vararg-pattern 8 drop-in-jgit
Branch vararg-pattern set up to track remote branch vararg-pattern from gosubpl.
Switched to a new branch 'vararg-pattern'
This is not urgent to fix, master is picked up. We can open an issue to fix this separately.
} | ||
} | ||
|
||
val fixFiles: Configured[Seq[FixFile]] = | ||
val fixFiles: Configured[Seq[FixFile]] = diffDisable.andThen(diffDisable0 => | ||
resolvedPathMatcher.andThen { pathMatcher => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use diffDisable.product(resolvedPathMatcher)
or |@|
instead of product
|
||
val oldTree1 = ref(repository, baseBranchRef) | ||
val newTree1 = new FileTreeIterator(repository) | ||
if (!branches.isEmpty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be more readable to have the Configured.error branch come first
|
||
private def fromRange(xs: List[(Int, Int)]): BitSet = { | ||
val n = 64 | ||
val max = xs.maxBy(_._2)._2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle the case when xs
is empty
scalafix-nightly -r DisableSyntax --config-str="DisableSyntax.keywords=[def]" --diff-branch=master
error: java.lang.UnsupportedOperationException: empty.maxBy
scala.collection.TraversableOnce.maxBy(TraversableOnce.scala:236)
This works now, even with remote branch:
|
Ready for round 4! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You outdid yourself @MasseGuillaume! Very nice job, I tested locally it works great, LGTM 👍
try { | ||
Option(repo.resolve(revstr)) match { | ||
case Some(id) => Right(id) | ||
case None => Left(s"cannot resolve $revstr") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot resolve --diff-base $revstr
No description provided.