-
Notifications
You must be signed in to change notification settings - Fork 185
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
Improve regex integration with DisableSyntax #907
Conversation
My current example is the |
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 looks great @Jacoby6000 Thank you for this contribution! I left a few minor comments, otherwise great job 👍
docs/rules/DisableSyntax.md
Outdated
] | ||
``` | ||
|
||
1. The first way has a simple object providing an `id`, `pattern`, and `message`. |
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.
s/simple//
docs/rules/DisableSyntax.md
Outdated
|
||
### Error Messages | ||
|
||
Error messages have access to the capture groups of the regex. Simply use `{$n}` |
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.
To access to the capture groups of the regex use
{$n}
...
@@ -52,8 +60,12 @@ case object DisableSyntaxBase { | |||
def -(other: String): String = s"$value - $other" | |||
} | |||
|
|||
5 // assert: DisableSyntax.magicNumbers |
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 assert the contents of the message here?
5 /* assert: DisableSyntax.magicNumbers
^
Numbers (5 in this instance) should always ...
*/
See end of this section https://scalacenter.github.io/scalafix/docs/developers/tutorial.html#use-diagnostic-to-report-linter-errors
Thanks for the feedback. I believe I've addressed all the comments |
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.
LGTM 👍 I thought this has already been merged a long time ago, sorry for the slow response @Jacoby6000 I'm releasing a new version today with this change 😊
fix docs: reflect new param signature after #907
I think the best way to see what I've changed is to look at the updated docs. To summarize:
I'm not dead set on any of the naming or formatting I've chosen. Feel free to have me change them