[TIMOB-24009] Properly generate properties and methods from protocols #121
Conversation
To skip unnecessary merging it will be checked if a parent class already implemented a protocol. The function doing this check contained a bug which would also skip the property and method merging even if the protocol wasn't implemented by parent class.
metabase/ios/lib/generate/index.js
Outdated
| * | ||
| * @param {Object} json Native code metabase | ||
| * @param {Object} cls Class to traverse upwards from | ||
| * @param {String} proto The protocoll to look for in parent classes |
metabase/ios/lib/generate/index.js
Outdated
| * @return {bool} True if protocol already implemented in a parent class, false otherwise. | ||
| */ | ||
| function isProtocolImplementedBySuperClass (json, cls, proto) { | ||
| var currentClass = cls.superclass; |
There was a problem hiding this comment.
This was checked against null before. Are there no cases where cls could be null?
There was a problem hiding this comment.
No because in the previous implementation cls was overwritten by cls.superclass and that could be undefined once NSObject was reached. Nevertheless i'll make an additional check just to be sure ;)
|
CR looks good. Will FT next, is the FB example supposed to work with this change? |
|
Yes, i was able to successfully call all methods on |
metabase/ios/lib/generate/index.js
Outdated
| var logIntendation = logIntendationCharacter.repeat(logIntendationLevel++); | ||
| var parentProtocols = protocol.protocols; | ||
| var protocolSignature = parentProtocols ? protocol.name + ' <' + parentProtocols.join(', ') + '>' : protocol.name; | ||
| util.logger.trace(logIntendation + 'Processing incorporated protocols for ' + protocolSignature); |
There was a problem hiding this comment.
"incorporate" sounds wrong here. Rather use "integrate" or "merge".
There was a problem hiding this comment.
I couldn't really decide on the wording here. Sometimes Apple refers to it as inherited protocols, sometimes as incorporated protocols. But i do agree incorporated sound odd here, i'll reword that.
metabase/ios/lib/generate/index.js
Outdated
| return; | ||
| } | ||
| if (!parentProtocols) { | ||
| util.logger.trace(logIntendation + protocol.name + ' does not incoporates any other protocols.'); |
There was a problem hiding this comment.
Remove the "s" from the verb, the 3rd person singular is already in "does" 😅.
metabase/ios/Gruntfile.js
Outdated
| }, | ||
| src: ['*.js','lib/**/*.js','test/**/*.js'] | ||
| src: ['*.js', 'lib/**/*.js'], |
There was a problem hiding this comment.
What happened to the test-files? And why the comma at the end?
There was a problem hiding this comment.
Most of the test files don't pass jshint because of the way should assertions are written, that's why i removed them from the list. The comma is leftover from when i used more options during testing, i'll fix that.
|
Addressed! |
|
Approved! |
JIRA: https://jira.appcelerator.org/browse/TIMOB-24009
To skip unnecessary merging it will be checked if a parent class
already implemented a protocol. The function doing this check contained
a bug which would also skip the property and method merging even if the
protocol wasn't implemented by parent class.