Ignore count of parameters in init methods#841
Closed
delebedev wants to merge 3 commits intorealm:masterfrom
delebedev:ignore-parameters-count-init
Closed
Ignore count of parameters in init methods#841delebedev wants to merge 3 commits intorealm:masterfrom delebedev:ignore-parameters-count-init
delebedev wants to merge 3 commits intorealm:masterfrom
delebedev:ignore-parameters-count-init
Conversation
Current coverage is 83.26% (diff: 100%)@@ master #841 diff @@
==========================================
Files 114 114
Lines 5136 5144 +8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 4275 4283 +8
Misses 861 861
Partials 0 0
|
Contributor
|
As far as I tested, value of "key.name" has prefix func `init`(…) {…}Output of |
Contributor
Author
|
@norio-nomura I've added the test case for scenario you've described and surprisingly it behaves as it should be. |
norio-nomura
suggested changes
Oct 16, 2016
Contributor
norio-nomura
left a comment
norio-nomura
left a comment
There was a problem hiding this comment.
Sorry for my unclear meaning in last comment with.
Please check my reviews. 🙏
| let length = Int(dictionary["key.namelength"] as? Int64 ?? 0) | ||
| let substructure = dictionary["key.substructure"] as? [SourceKitRepresentable] ?? [] | ||
|
|
||
| if file.contents.substring(nameOffset, length: length).containsString("init(") { |
Contributor
There was a problem hiding this comment.
nameOffsetis counted by byte based number.
So, this make failing test with triggering example such as:
"struct Foo {\n /* 👨👩👧👦👨👩👧👦👨👩👧👦 */" +
"init(a: Int, b: Int, c: Int, d: Int, e: Int, f: Int) {}\n" +
"func bar(a: b, c: Int, d: Int, e: Int, f: Int, g: Int) {}}"Please use SourceKittenFrameworks's byte base API such as NSString.substringWithByteRange(start:length:) if getting substring from file.contents.
- This also fails with
init?(.
What I meant on my last comment were:
- "key.name" already contains string what we need here.
Most of offset conversion between byte and utf16 are relatively high cost operations. We want to avoid using that if we can. - And that has prefix
init(whether initializer is failable or not
Collaborator
|
@garnett thanks for your work on this. If you could address @norio-nomura's last comment, I think we could get this in! 💃 |
Collaborator
|
Also a changelog entry could be helpful. |
Contributor
Author
|
@jpsim I've rebased original PR and tried to resolve all comments. |
marcelofabri
reviewed
Dec 6, 2016
| } | ||
|
|
||
| fileprivate func functionIsInitializer(_ file: File, offset: Int, length: Int) -> Bool { | ||
| if let function = (file.contents as NSString) |
Collaborator
There was a problem hiding this comment.
I think this cast is not needed
Contributor
Author
There was a problem hiding this comment.
If I understood correctly, @norio-nomura was explicit in his comment and suggested using NSString. If you search for (file.contents as NSString) in the project you'll see that sometimes this conversion is used even if redundant. But I can be wrong here.
Collaborator
There was a problem hiding this comment.
this might matter when we add Linux support, but the implicit bridging works fine for now and is cleaner, so let's stick with the cleaner approach.
marcelofabri
reviewed
Dec 6, 2016
|
|
||
| fileprivate func functionIsInitializer(_ file: File, offset: Int, length: Int) -> Bool { | ||
| if let function = (file.contents as NSString) | ||
| .substringWithByteRange(start: offset, length: length), function.contains("init") { |
Collaborator
There was a problem hiding this comment.
maybe check hasPrefix instead?
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #544
As was mentioned deleting
FunctionConstructorfrom the list does not really work.My solution looks pretty dumb, but those two tests prove that it works. I've decided to test for
init(instead ofinitto cut at least some of false positives (such asinitProperties) but arguably it is not really necessary.