Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add id for CustomMessage (fix #495) #500

Merged
merged 2 commits into from
Dec 15, 2017
Merged

Add id for CustomMessage (fix #495) #500

merged 2 commits into from
Dec 15, 2017

Conversation

MasseGuillaume
Copy link
Contributor

No description provided.

MasseGuillaume
MasseGuillaume commented Dec 12, 2017
View reviewed changes
scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala Outdated
class CustomMessage[T](value: T, message: Option[String], id: Option[String])

object CustomMessage {
def getOption[T](obj: Conf.Obj, path: String)(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/olafurpg/metaconfig/pull/31

MasseGuillaume
MasseGuillaume commented Dec 12, 2017
View reviewed changes
scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala Outdated
ConfDecoder.instance[CustomMessage[T]] {
case obj: Conf.Obj => {
(obj.get[T](field) |@|
getOption[String](obj, "message") |@|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/olafurpg/metaconfig/pull/31

replace with obj.getOption[String]("message")

Copy link
Contributor

Choose a reason for hiding this comment

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

metaconfig v0.5.4 is out with getOption 🔥

MasseGuillaume
MasseGuillaume commented Dec 12, 2017
View reviewed changes
scalafix-tests/input/src/main/scala/test/DisableSyntax.scala
@@ -11,6 +11,7 @@ DisableSyntax.noSemicolons = true
DisableSyntax.noXml = true
DisableSyntax.regex = [
{
id = offensive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot use pimp here because the assert would trigger the regex again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I wonder if we can avoid this in general by making sure the offensive part does not follow scalafix:ok/off. Since we have the start offset, we should be able to handle this. I opened #507 to track this.

Copy link
Contributor

Choose a reason for hiding this comment

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

"offensive" is better anyways.

olafurpg
olafurpg suggested changes Dec 15, 2017
View reviewed changes
Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for going the extra mile and adding getOption upstream to metaconfig 👍 Just a few minor comments, otherwise looks great

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala Outdated
ConfDecoder.instance[CustomMessage[T]] {
case obj: Conf.Obj => {
(obj.get[T](field) |@|
getOption[String](obj, "message") |@|
Copy link
Contributor

Choose a reason for hiding this comment

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

metaconfig v0.5.4 is out with getOption 🔥

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala

import scala.language.implicitConversions

class CustomMessage[T](
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this is in our public API, can we add

type CustomMessage[T] = scalafix.config.CustomMessage[T]
val CustomMessage = scalafix.config.CustomMessage

in the scalafix package object? Rule authors should only need to import scalafix._; import scala.meta._ to get started.

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala
class CustomMessage[T](
val value: T,
val message: Option[String],
val id: Option[String])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add withValue/withMessage/withId helpers to make it more convenient to copy one field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You only read from the CustomMessage, I don't think this is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I agree.

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala
val message: Option[String],
val id: Option[String])

object CustomMessage {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a apply(value: T) = new CustomMessage(value, None, None) constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ibid. you only read from the conf.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, nevermind!

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala Outdated
val id: Option[String])

object CustomMessage {
def getOption[T](obj: Conf.Obj, path: String)(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove this since it's public API now.

scalafix-tests/input/src/main/scala/test/DisableSyntax.scala
@@ -11,6 +11,7 @@ DisableSyntax.noSemicolons = true
DisableSyntax.noXml = true
DisableSyntax.regex = [
{
id = offensive
Copy link
Contributor

Choose a reason for hiding this comment

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

"offensive" is better anyways.

@MasseGuillaume
Copy link
Contributor Author

Ready for round 2.

olafurpg
olafurpg approved these changes Dec 15, 2017
View reviewed changes
Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala
class CustomMessage[T](
val value: T,
val message: Option[String],
val id: Option[String])
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I agree.

scalafix-core/shared/src/main/scala/scalafix/config/CustomMessage.scala
val message: Option[String],
val id: Option[String])

object CustomMessage {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, nevermind!

scalafix-core/shared/src/main/scala/scalafix/internal/config/DisableConfig.scala
@@ -1,4 +1,5 @@
package scalafix.internal.config
package scalafix
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally avoid this pattern since it can easily introduce cycles in your codebase. import scalafix.config.CustomMessage is fine in our own code. However, I'm not so consistent myself 😸

@olafurpg olafurpg merged commit a2c5d70 into scalacenter:master Dec 15, 2017
MasseGuillaume added a commit to MasseGuillaume/scalafix that referenced this pull request Dec 20, 2017
@MasseGuillaume
Working diff-base completion
b4c4996 
```
> scalafix --diff-base <tab>
HEAD
ag/gh-pages
ag/issue-260
ag/issue-319
ag/master
...
v0.5.5
v0.5.6
|00| 51abefa -- wip improve sbt completion (13 hours ago)
|01| 3f63b5b -- Merge pull request scalacenter#474 from marcelocenerine/trim_includeNewLine (13 hours ago)
|02| 47b94c0 -- Add leadingSpaces/trailingSpaces methods to TokenList (13 hours ago)
|03| 8bd026d -- Merge pull request scalacenter#503 from MasseGuillaume/feature/368-escape-patches (13 hours ago)
|04| 20e445f -- Escape hatch on Patch (fix scalacenter#368) (13 hours ago)
|05| a2c5d70 -- Merge pull request scalacenter#500 from MasseGuillaume/feature/495-custom-id (13 hours ago)
|06| 59efe7d -- Add CustomMessage to the public api (13 hours ago)
|07| 9ae6071 -- Add id for CustomMessage (fix scalacenter#495) (13 hours ago)
|08| e4a5c35 -- Merge pull request scalacenter#494 from MasseGuillaume/disable-regex (13 hours ago)
|09| a422860 -- Merge pull request scalacenter#497 from olafurpg/disable-signatures (13 hours ago)
|10| 7930947 -- DisableSyntax add regex (13 hours ago)
|11| 5dbdd6b -- IntervalSet test for empty and add toString (13 hours ago)
|12| b022fbd -- DisableSyntax don't repeat DisableSyntax.keyword in message (13 hours ago)
|13| a992b02 -- Assert instead of scalafix:ok (13 hours ago)
|14| 7896ccd -- Refactor Disable to use views. (13 hours ago)
|15| 58acdbe -- Fix scalacenter#493, handle synthetics and symbol signatures in Disable. (13 hours ago)
|16| b48d7f0 -- Merge pull request scalacenter#490 from olafurpg/unmanagedSources (13 hours ago)
|17| e9b2b0a -- s/canFormat/canFix/ (13 hours ago)
|18| 26be6fa -- Use unmanagedSources instead of unmanagedSourceDirectories. (13 hours ago)
|19| 4d46001 -- Merge pull request scalacenter#488 from olafurpg/master (13 hours ago)
```
MasseGuillaume added a commit to MasseGuillaume/scalafix that referenced this pull request Dec 22, 2017
@MasseGuillaume
Working diff-base completion
0fc821c 
```
> scalafix --diff-base <tab>
HEAD
ag/gh-pages
ag/issue-260
ag/issue-319
ag/master
...
v0.5.5
v0.5.6
|00| 51abefa -- wip improve sbt completion (13 hours ago)
|01| 3f63b5b -- Merge pull request scalacenter#474 from marcelocenerine/trim_includeNewLine (13 hours ago)
|02| 47b94c0 -- Add leadingSpaces/trailingSpaces methods to TokenList (13 hours ago)
|03| 8bd026d -- Merge pull request scalacenter#503 from MasseGuillaume/feature/368-escape-patches (13 hours ago)
|04| 20e445f -- Escape hatch on Patch (fix scalacenter#368) (13 hours ago)
|05| a2c5d70 -- Merge pull request scalacenter#500 from MasseGuillaume/feature/495-custom-id (13 hours ago)
|06| 59efe7d -- Add CustomMessage to the public api (13 hours ago)
|07| 9ae6071 -- Add id for CustomMessage (fix scalacenter#495) (13 hours ago)
|08| e4a5c35 -- Merge pull request scalacenter#494 from MasseGuillaume/disable-regex (13 hours ago)
|09| a422860 -- Merge pull request scalacenter#497 from olafurpg/disable-signatures (13 hours ago)
|10| 7930947 -- DisableSyntax add regex (13 hours ago)
|11| 5dbdd6b -- IntervalSet test for empty and add toString (13 hours ago)
|12| b022fbd -- DisableSyntax don't repeat DisableSyntax.keyword in message (13 hours ago)
|13| a992b02 -- Assert instead of scalafix:ok (13 hours ago)
|14| 7896ccd -- Refactor Disable to use views. (13 hours ago)
|15| 58acdbe -- Fix scalacenter#493, handle synthetics and symbol signatures in Disable. (13 hours ago)
|16| b48d7f0 -- Merge pull request scalacenter#490 from olafurpg/unmanagedSources (13 hours ago)
|17| e9b2b0a -- s/canFormat/canFix/ (13 hours ago)
|18| 26be6fa -- Use unmanagedSources instead of unmanagedSourceDirectories. (13 hours ago)
|19| 4d46001 -- Merge pull request scalacenter#488 from olafurpg/master (13 hours ago)
```
@MasseGuillaume MasseGuillaume added this to the v0.5.8 milestone Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olafurpg olafurpg olafurpg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.5.8
Development

Successfully merging this pull request may close these issues.
2 participants
@MasseGuillaume @olafurpg