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.

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

Add configuration for trailing_whitespace to ignore comments #854

Merged
merged 5 commits into from
Nov 4, 2016
Merged

Add configuration for trailing_whitespace to ignore comments #854

merged 5 commits into from
Nov 4, 2016

Conversation

jaherhi
Copy link
Contributor

@jaherhi jaherhi commented Oct 22, 2016

Fixes #576

Rule trailing_whitespace can now be configured to ignore comments using ignores_comments, its default value is true

@codecov-io
Copy link

codecov-io commented Oct 22, 2016

Current coverage is 85.67% (diff: 95.74%)

Merging #854 into master will increase coverage by 0.17%

@@             master       #854   diff @@
==========================================
  Files           108        108          
  Lines          4690       4767    +77   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4010       4084    +74   
- Misses          680        683     +3   
  Partials          0          0          

Powered by Codecov. Last update e8771d9...c1df008

@marcelofabri
Copy link
Collaborator

I think you should use SyntaxKinds instead of checking for //. Also, I think this implementation won't work in "real" examples because it only checks the beginning of strings, i.e. this would fail:

class Example {
  // empty but with trailing spaces          
}

You probably can use something like this to check if the line only contains comments instead.

@jaherhi
Copy link
Contributor Author

jaherhi commented Oct 23, 2016

Thanks for the suggestion @marcelofabri! I managed to change my implementation to use SyntaxKind, although I'm afraid I don't get what you mean with the example, and I'm trying to see if that could happen using SyntaxKind too.

In your example, checking for // at the beginning of the string would detect that second line as a comment and, by default, ignore it even if it has trailing spaces, wouldn't it?

Copy link
Collaborator

@norio-nomura norio-nomura left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! 🙏
It looks there is some performance regression that may be resolvable.
Could you please check my feedbacks?

I also found false negative pattern with block comment:

  let nsError = NSError(domain: NSCocoaErrorDomain,
                   code: NSFileNoSuchFileError,
                   userInfo: [
                     AnyHashable(NSFilePathErrorKey) : "/dev/null",
/* this line */      AnyHashable(NSStringEncodingErrorKey): /*ASCII=*/1,    
                   ])

)

public func validateFile(file: File) -> [StyleViolation] {
let filteredLines = file.lines.filter {
$0.content.hasTrailingWhitespace() &&
let commentKinds = Set(SyntaxKind.commentKinds())
Copy link
Collaborator

Choose a reason for hiding this comment

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

commentKinds is not need to be Set.

$0.content.hasTrailingWhitespace() &&
let commentKinds = Set(SyntaxKind.commentKinds())
let lineSyntaxKinds = file.syntaxKindsByLines[$0.index]
let lineContainsComment = !lineSyntaxKinds.filter { commentKinds.contains($0) }.isEmpty
Copy link
Collaborator

Choose a reason for hiding this comment

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

lineContainsComment should be calculated only if $0.content.hasTrailingWhitespace() and configuration.ignoresComments are true.
Please pay attention to reduce calculation. 🕵🏻

@jaherhi
Copy link
Contributor Author

jaherhi commented Nov 1, 2016

Changes made to address the feedback 🙂 Thank you!

Copy link
Collaborator

@norio-nomura norio-nomura left a comment

Choose a reason for hiding this comment

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

I found false negative pattern that block comment has trailing whitespace:

let string1 = "string1"  /* block comment has trailing whitespace */  

But, maybe that won't be problems. 😃

So, it looks mostly good to me other than my comments.
Could you please check them?

return false
}

return $0.content.hasTrailingWhitespace() &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

On non violating case, $0.content.hasTrailingWhitespace() is evaluated twice. The closure should return false first if $0.content.hasTrailingWhitespace() returns false.

@@ -46,6 +55,14 @@ public struct TrailingWhitespaceRule: CorrectableRule, ConfigurationProviderRule
var corrections = [Correction]()
let fileRegions = file.regions()
for line in file.lines {
let commentKinds = SyntaxKind.commentKinds()
if line.content.hasTrailingWhitespace() && configuration.ignoresComments,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with my comment on validateFile(_:).
This loop does not need to evaluate following operations if line.content.hasTrailingWhitespace() returns false.

if !line.content.hasTrailingWhitespace() {
    correctedLines.append(line.content)
    continue
}

@jaherhi
Copy link
Contributor Author

jaherhi commented Nov 3, 2016

Changes made. Thanks for your comments and patience, @norio-nomura!

I can't think of a way to fix that without manually checking if the comment contains */and whitespace after it. Any suggestion, if that's a case that should work properly?

@norio-nomura
Copy link
Collaborator

We can continue with filing issue of the false negative and another PR. 😉
I want to merge this PR. But I forgot about CHANGELOG.md.
Could you please add a changelog entry, following the guidelines in CONTRIBUTING.md?

@jaherhi
Copy link
Contributor Author

jaherhi commented Nov 4, 2016

CHANGELOG.mdupdated. I rebased the branch to avoid conflicts after updating it.

@norio-nomura norio-nomura merged commit 4ebe939 into realm:master Nov 4, 2016
@norio-nomura
Copy link
Collaborator

Thanks! 🙏

@jpsim
Copy link
Collaborator

jpsim commented Nov 25, 2016

Awesome! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants