-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix crash on method names that break regex patterns #12
Conversation
Note, this fix also provides a fix to let the tests run on backwards windows path separators. |
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.
Thanks! I'll take a closer look tonight and merge it.
import scala.meta._ | ||
|
||
import co.pjrt.stags.paths.Path | ||
|
||
object TagGenerator { | ||
|
||
|
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's with all the empty new lines?
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.
Sorry, the empty lines are remnants of some debug harness that I removed; but didn't catch the empty lines.
@@ -160,6 +161,9 @@ object TagGenerator { | |||
// DESNOTE(2017-07-14, pjrt): This is for patterns like `val 1 = x` | |||
// For those, we should not generate tags (of course) | |||
Seq.empty |
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.
👍
.toString shouldBe expectedS | ||
|
||
.toString | ||
.replaceAllLiterally("\\","/") //windows path sep is backwards |
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 don't understand this change
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 get that, I don't get why this was needed. Are u running this test on windows and it is failing?
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 was testing this on a Windows machine. In order to make the test work, I had to make the backward paths match the tests specified in a unix style path.
the replaceAllLiterally
is just wedged in before the shouldBe
.
Well the code formatter isn't set up in SBT, and I have no idea whether u can get it to run on scalafmt, so I'll run it myself post-merge. Thanks! |
Note, this pull request was not specifically tested. However, it has been tested in the branch
drxcc/stags/dep-updates
which bumps the project to sbt 1.0.These changes fix tag generation for a broader number of repositories including
scala/scala
andsbt/sbt
.