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 class properties by parsing the good tag #285

Closed
wants to merge 2 commits into from
Closed

fix class properties by parsing the good tag #285

wants to merge 2 commits into from

Conversation

jeremiegirault
Copy link
Contributor

@jeremiegirault jeremiegirault commented Oct 27, 2016

handles #243

I don't know how sourcekitten works so this must be a dirty patch, but I had to fix it quickly to make jazzy work for me.

Better solutions are welcome but it's working (not crashing at least) and is a good starting point.

@jpsim
Copy link
Owner

jpsim commented Oct 27, 2016

Thanks for investigating this and finding a fix! This will need a bit more work before it can land, though.

First, I'd like to see a unit test that covers this change. For example, the existing ClangTranslationUnitTests.testBasicObjectiveCDocs() test could be extended to cover this by adding the following to the Musician.h fixture:

 #pragma mark - Properties

+/// Does this class model musicians?
+@property (class, readwrite, nonatomic) BOOL isMusician;
+
 /**
  The name of the musician. i.e. "John Coltrane"
  */

By applying this change, I noticed that clang generates 3 declarations when declaring a readwrite property (or 2 if readonly):

{
  "key.filepath" : "Musician.h",
  "key.doc.file" : "Musician.h",
  "key.deprecation_message" : "",
  "key.always_unavailable" : false,
  "key.unavailable_message" : "",
  "key.parsed_scope.start" : 17,
  "key.kind" : "sourcekitten.source.lang.objc.decl.property",
  "key.swift_declaration" : "class var isMusician: Bool { get set }",
  "key.doc.full_as_xml" : "",
  "key.always_deprecated" : false,
  "key.doc.line" : 17,
  "key.doc.column" : 46,
  "key.name" : "isMusician",
  "key.doc.comment" : "Always returns `YES`.",
  "key.usr" : "c:objc(cs)JAZMusician(cpy)isMusician",
  "key.parsed_declaration" : "@property (assign, readwrite, nonatomic, class) BOOL isMusician;",
  "key.parsed_scope.end" : 17
},
{
  "key.filepath" : "Musician.h",
  "key.doc.file" : "Musician.h",
  "key.deprecation_message" : "",
  "key.always_unavailable" : false,
  "key.unavailable_message" : "",
  "key.parsed_scope.start" : 17,
  "key.kind" : "sourcekitten.source.lang.objc.decl.method.class",
  "key.always_deprecated" : false,
  "key.doc.line" : 17,
  "key.doc.column" : 46,
  "key.name" : "+isMusician",
  "key.usr" : "c:objc(cs)JAZMusician(cm)isMusician",
  "key.parsed_declaration" : "+ (BOOL)isMusician;",
  "key.parsed_scope.end" : 17
},
{
  "key.filepath" : "Musician.h",
  "key.doc.file" : "Musician.h",
  "key.deprecation_message" : "",
  "key.always_unavailable" : false,
  "key.unavailable_message" : "",
  "key.parsed_scope.start" : 17,
  "key.kind" : "sourcekitten.source.lang.objc.decl.method.class",
  "key.always_deprecated" : false,
  "key.doc.line" : 17,
  "key.doc.column" : 46,
  "key.name" : "+setIsMusician:",
  "key.usr" : "c:objc(cs)JAZMusician(cm)setIsMusician:",
  "key.parsed_declaration" : "+ (void)setIsMusician:(BOOL)isMusician;",
  "key.parsed_scope.end" : 17
}

So that's the property, along with getter and setter methods. For instance properties, we reject implicitly generated property getters & setters (see Sequence.rejectPropertyMethods()).

However, because accessorUSR(getter:) returns incorrect results for class properties, this doesn't work for those. It generates incorrect results for a few reasons:

  • You'll notice in the JSON snippet I've shared above, that the USR uses (cm) to indicate a "class method" rather than the (im) you're using, which denotes an "instance method". The fix here is to change replacingOccurrences(of: "(cpy)", with: "(im)") to replacingOccurrences(of: "(cpy)", with: "(cm)")
  • restOfSetterName is incorrect because restOfSetterName assumes a length of (py) (4), which is incorrect for (cpy) (5).

I've pushed up my partial work to address these issues in 6499d8b, but that could certainly be improved by some refactoring. @jeremiegirault if you're up for it, I'll let you continue your work in this PR to address the points I've brought up.

Thanks again for the contribution.

@jeremiegirault
Copy link
Contributor Author

I'll look at this.

@jpsim
Copy link
Owner

jpsim commented Nov 1, 2016

Let me know if you have any questions or would like any help.

@jeremiegirault
Copy link
Contributor Author

sure !

@jeremiegirault
Copy link
Contributor Author

your fix looks good, just rebased your master and updated some fixtures to have tests passing, is it good ?

@jpsim
Copy link
Owner

jpsim commented Nov 1, 2016

Looks like you made some changes to account for Xcode 8.1 and Swift 3.0.1, but since the unit tests still run on 8.0 on Travis, they're now failing.

@jeremiegirault
Copy link
Contributor Author

ok then I'll revert this

@jeremiegirault
Copy link
Contributor Author

done

@jpsim
Copy link
Owner

jpsim commented Nov 3, 2016

Thanks for your hard work on this @jeremiegirault! Will merge in #291.

@jpsim jpsim closed this Nov 3, 2016
@jeremiegirault
Copy link
Contributor Author

haha it was mostly your work @jpsim but thank you :)

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