-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Color literals count as single character towards line count #830
Conversation
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 for this @VFUC, the changes mostly look good.
Could you please fix my feedback and add following?
- examples containing color literal in
nonTriggeringExamples
andtriggeringExamples
- an enhancement entry in CHANGELOG.md
locale: nil) { | ||
|
||
// Range was found, get substring of color literal range | ||
let colorLiteralContent = line.content.substringWithRange(colorLiteralRangeStart.startIndex..<colorLiteralRangeEnd.endIndex) |
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 detect multiple color literals in one line
…d as any other characters
Thanks for the feedback, I've added the requested changes. I have also modified the general approach to support multiple color literals in a single line. |
Current coverage is 84.01% (diff: 96.15%)@@ master #830 diff @@
==========================================
Files 101 101
Lines 4035 4060 +25
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 3387 3411 +24
- Misses 648 649 +1
Partials 0 0
|
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 for changes.
I have also modified the general approach to support multiple color literals in a single line.
Looks good to me 👍 other than indentation and 2 trailing spaces.
options: .LiteralSearch, | ||
range: colorLiteralRangeStart.startIndex..<line.content.endIndex, | ||
range: rangeStart.startIndex..<string.endIndex, | ||
locale: nil) { |
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 you please add indent to lines between let rangeEnd…
and locale:nil) {
?
@@ -17,6 +17,10 @@ | |||
[Norio Nomura](https://github.com/norio-nomura) | |||
[Syo Ikeda](https://github.com/ikesyo) | |||
|
|||
* Color literals count as single characters to avoid unintentional line length violations. |
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 you please add 2 trailing spaces to this line for making Markdown adds line ending?
Maybe this should be improved to handle other literals (such as image ones)? |
I have now extracted the literal stripping into a function and applied it for color and image literals (as suggested by @marcelofabri , thanks for that!). If future literals keep the same I've also added the requested formatting. |
Thanks for doing this! 🙏 |
Hey! Trying to work on issue #742 - I've added a check for an expanded color literal substring when counting line characters. If found, it subtracts the length of the substring (-1) from the total line length, so that a color literal only counts as a single character towards the line count rule. Feedback welcome 🙂