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

Fix ignore identifiers starting with '@' #197

Merged
merged 1 commit into from
Apr 7, 2016

Conversation

norio-nomura
Copy link
Collaborator

isIdentifier always returned false.

`isIdentifier` always returned false.
@jpsim jpsim merged commit 1141bb2 into master Apr 7, 2016
@jpsim jpsim deleted the nn-fix-ignore-identifiers-starting-with-at branch April 7, 2016 14:28
@jpsim
Copy link
Owner

jpsim commented Apr 7, 2016

Thanks!

@norio-nomura
Copy link
Collaborator Author

🙏

@jpsim
Copy link
Owner

jpsim commented Apr 18, 2016

Hey @norio-nomura, do you remember why you did this? In realm/SwiftLint#608 you said that otherwise Swift 2.2 tests failed, but I can't reproduce that, and now we just discovered that this introduced a regression in jazzy: realm/jazzy#530

I obviously don't want to revert this if it will cause other regressions, but I don't know what those are 😄

@jpsim
Copy link
Owner

jpsim commented Apr 18, 2016

Never mind, I can reproduce the failures... I'll have to see how to fix one bug without reintroducing the other!

@norio-nomura
Copy link
Collaborator Author

Sorry for missing documentation.
That fixes "Stop searching comment on identifier not having @ prefix." to as is.
Before fix, that never stop searching on any identifiers.

@norio-nomura
Copy link
Collaborator Author

@jpsim I want to see the regression of realm/jazzy#530. Where can I check that?

@jpsim
Copy link
Owner

jpsim commented Apr 19, 2016

You can see the diff to jazzy's generated output here: https://github.com/realm/jazzy-integration-specs/compare/jp-render-swift-declarations-in-objc

The regression is the lost extension documentation in Moya.

I have some ideas on how to fix this for both SwiftLint and jazzy but it's a bit complex. It involves checking the structure depth of the previous documentation comment compared to the documented declaration, and validating the structures in between.

@norio-nomura
Copy link
Collaborator Author

Thanks for information.
I see following.

/// Extension for processing raw NSData generated by network access.
@available(*, deprecated, message="This will be removed when ReactiveCocoa 4 becomes final. Please visit https://github.com/Moya/Moya/issues/298 for more information.")
public extension RACSignal {

Now I know the reason why you added checking .Extension on 385115a#diff-c40f53a6b61685c6741ff5753303e59aR324.

I think we need to detect attributes much smarter.
I'll have a look about another approach on detecting attribute.

@norio-nomura
Copy link
Collaborator Author

I'll have a look about another approach on detecting attribute.

That seems difficult.
How about separating getDocumentationCommentBody() into SwiftLint version and jazzy version?
On jazzy only version, we can use approach of #142 (comment)

@norio-nomura
Copy link
Collaborator Author

I came up with another idea that use parent structure's body offset or sibling structure's end offset for limiting search doc comment.
I'll try to implement that.

@jpsim
Copy link
Owner

jpsim commented Apr 19, 2016

I came up with another idea that use parent structure's body offset or sibling structure's end offset for limiting search doc comment.

Yeah, this is along the lines of what I meant in #197 (comment). There's no rush for this, if you don't get around to doing this, I'll probably find time at some point, just to get realm/jazzy#530 out.

@norio-nomura
Copy link
Collaborator Author

It seems my fix is not needed.
Thanks!

@jpsim
Copy link
Owner

jpsim commented Apr 21, 2016

yeah I fixed this in #95, sorry for not updating this thread when that happened! 😬

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.

2 participants