From e6531ae380c1540252c5c751a730d7e511b5e1ee Mon Sep 17 00:00:00 2001 From: Chris Rebert Date: Sun, 29 Nov 2015 01:02:58 -0800 Subject: Fix #33 --- docs/newfile.md | 7 ++++ .../rorschach/auditing/AddedFilesAuditor.scala | 24 ++++++++++++ .../rorschach/github/util/package.scala | 3 ++ .../rorschach/server/PullRequestEventHandler.scala | 20 +++++----- .../com/getbootstrap/rorschach/util/package.scala | 43 ++++++++++++++++++++++ 5 files changed, 87 insertions(+), 10 deletions(-) create mode 100644 docs/newfile.md create mode 100644 src/main/scala/com/getbootstrap/rorschach/auditing/AddedFilesAuditor.scala diff --git a/docs/newfile.md b/docs/newfile.md new file mode 100644 index 0000000..8c3636a --- /dev/null +++ b/docs/newfile.md @@ -0,0 +1,7 @@ +This warning is triggered if a pull request adds a file named `new-file.txt` (or similar). + +Such a filename is indicative of pull requests which were created to test out Git or GitHub, rather than proposing any legitimate change to Bootstrap. + +If you want to test out Git or GitHub, **[create your own personal repository](https://guides.github.com/activities/hello-world/) and conduct your experiments there instead**, where they won't bother anyone. Using the repositories of public projects (such as Bootstrap) for such experiments wastes the time of those projects' maintainers and is thus considered rude & annoying. Which is why your pull request was automatically closed. + +To learn more about using Git or GitHub, check out the links on the following GitHub Help page: https://help.github.com/categories/bootcamp/ diff --git a/src/main/scala/com/getbootstrap/rorschach/auditing/AddedFilesAuditor.scala b/src/main/scala/com/getbootstrap/rorschach/auditing/AddedFilesAuditor.scala new file mode 100644 index 0000000..5a53206 --- /dev/null +++ b/src/main/scala/com/getbootstrap/rorschach/auditing/AddedFilesAuditor.scala @@ -0,0 +1,24 @@ +package com.getbootstrap.rorschach.auditing + +import com.getbootstrap.rorschach.util._ + +object AddedFilesAuditor { + private val message = + """[The fact that this pull request adds a new file named something like `new-file.txt` indicates that you were using Bootstrap to test out Git or GitHub, rather than proposing a legitimate change.](https://github.com/twbs/rorschach/blob/master/docs/newfile.md) + |If that's accurate, please **DON'T** do that again! + |Instead, [use *your own personal repositories*](https://guides.github.com/activities/hello-world/) to experiment with [how to use Git or GitHub](https://help.github.com/articles/good-resources-for-learning-git-and-github/). + |Using the repos of public projects (such as Bootstrap) for such experiments wastes the time of those projects' maintainers + |and is thus considered rude.""".stripMargin.replaceAllLiterally("\n", " ") + + def audit(addedFilepaths: Set[String]): Seq[String] = { + val filenames = addedFilepaths.map{ filepath => { + filepath.onlyFilename.withoutExtension.replaceAllLiterally("-", "").replaceAllLiterally(" ", "").asciiLowerCased + }} + if (filenames.contains("newfile")) { + Seq(message) + } + else { + Nil + } + } +} diff --git a/src/main/scala/com/getbootstrap/rorschach/github/util/package.scala b/src/main/scala/com/getbootstrap/rorschach/github/util/package.scala index 9a2aa6e..43db97c 100644 --- a/src/main/scala/com/getbootstrap/rorschach/github/util/package.scala +++ b/src/main/scala/com/getbootstrap/rorschach/github/util/package.scala @@ -20,4 +20,7 @@ package object util { } def status = PullRequestStatus(pr.getState) } + implicit class RichCommitFileSeq(files: Seq[CommitFile]) { + def filenames: Set[String] = files.map{ _.getFilename }.toSet[String] + } } diff --git a/src/main/scala/com/getbootstrap/rorschach/server/PullRequestEventHandler.scala b/src/main/scala/com/getbootstrap/rorschach/server/PullRequestEventHandler.scala index 724f96b..491b920 100644 --- a/src/main/scala/com/getbootstrap/rorschach/server/PullRequestEventHandler.scala +++ b/src/main/scala/com/getbootstrap/rorschach/server/PullRequestEventHandler.scala @@ -10,11 +10,10 @@ import com.getbootstrap.rorschach.github._ import com.getbootstrap.rorschach.github.util._ class PullRequestEventHandler(commenter: ActorRef) extends GitHubActorWithLogging { - private def modifiedFilesFor(repoId: RepositoryId, base: CommitSha, head: CommitSha) = { + private def affectedFilesFor(repoId: RepositoryId, base: CommitSha, head: CommitSha) = { val commitService = new CommitService(gitHubClient) Try { commitService.compare(repoId, base.sha, head.sha) }.map { comparison => - val affectedFiles = comparison.getFiles.asScala - affectedFiles.filter{ _.status == Modified }.map{ _.getFilename }.toSet[String] + comparison.getFiles.asScala } } @@ -38,18 +37,19 @@ class PullRequestEventHandler(commenter: ActorRef) extends GitHubActorWithLoggin val head = prHead.commitSha val foreignRepoId = prHead.getRepo.repositoryId - val titleMessages = TitleAuditor.audit(pr.getTitle) - val branchMessages = BaseAndHeadBranchesAuditor.audit(baseBranch = bsBase.getRef, headBranch = prHead.getRef) - val fileMessages = modifiedFilesFor(foreignRepoId, base, head) match { + val affectedFiles = affectedFilesFor(foreignRepoId, base, head) match { case Failure(exc) => { - log.error(exc, s"Could not get modified files for commits ${base}...${head} for ${foreignRepoId}") + log.error(exc, s"Could not get affected files for commits ${base}...${head} for ${foreignRepoId}") Nil } - case Success(modifiedFiles) => { - ModifiedFilesAuditor.audit(modifiedFiles) - } + case Success(files) => files } + val modifiedFiles = affectedFiles.filter{ _.status == Modified }.filenames + val addedFiles = affectedFiles.filter{ _.status == Added }.filenames + val titleMessages = TitleAuditor.audit(pr.getTitle) + val branchMessages = BaseAndHeadBranchesAuditor.audit(baseBranch = bsBase.getRef, headBranch = prHead.getRef) + val fileMessages = ModifiedFilesAuditor.audit(modifiedFiles) ++ AddedFilesAuditor.audit(addedFiles) val allMessages = titleMessages ++ branchMessages ++ fileMessages if (allMessages.nonEmpty) { commenter ! PullRequestFeedback(destinationRepo, pr.number, pr.getUser, allMessages) diff --git a/src/main/scala/com/getbootstrap/rorschach/util/package.scala b/src/main/scala/com/getbootstrap/rorschach/util/package.scala index 2844af5..a4cf38f 100644 --- a/src/main/scala/com/getbootstrap/rorschach/util/package.scala +++ b/src/main/scala/com/getbootstrap/rorschach/util/package.scala @@ -19,4 +19,47 @@ package object util { def asciiLowerCased: String = str.toLowerCase(Locale.ENGLISH) } + + implicit class SplittableString(str: String) { + /** + * Python's str.partition() + */ + def splitOnce(separator: String): (String, String) = { + str.lastIndexOf(separator) match { + case -1 => (str, "") + case sepStart => snipOut(sepStart, separator.length) + } + } + + /** + * Python's str.rpartition() + */ + def splitOnceFromRight(separator: String): (String, String) = { + str.lastIndexOf(separator) match { + case -1 => ("", str) + case sepStart => snipOut(sepStart, separator.length) + } + } + + private def snipOut(start: Int, length: Int): (String, String) = { + val prefix = str.substring(0, start) + val suffix = str.substring(start + length) + (prefix, suffix) + } + } + + implicit class FilepathString(filepath: String) { + def onlyFilename: String = filepath.splitOnceFromRight("/")._2 + } + + implicit class FilenameString(filename: String) { + def withoutExtension: String = { + if (filename.length >= 1 && filename.charAt(0) == '.') { + filename + } + else { + filename.splitOnce(".")._1 + } + } + } } -- cgit v1.2.3