Feature/omit non private modifiers on private types #1398
Conversation
|
If you accept this PR, please add the "hacktoberfest-accepted" label, thank you. |
| @@ -299,10 +300,10 @@ public class TypeSpec private constructor( | |||
| } | |||
|
|
|||
| // Functions. | |||
| for (funSpec in funSpecs) { | |||
| for (funSpec in funSpecsOmittingVisibility()) { | |||
There was a problem hiding this comment.
Not sure it makes sense to filter out the modifiers both here and when passing the implicit modifiers to funSpec.emit() - maybe we should simply add unnecessary visibility modifiers to implicitModifiers if the type is private? Any reason why that wouldn't work?
There was a problem hiding this comment.
Hopefully that also adds support for properties which I suspect exhibit the same behavior.
There was a problem hiding this comment.
Honestly, I don't know the library this deep... I tried to reverse engineer how the mechanism works and found out that filtering here before emit() caused the least amount of code changes. What would you suggest as a better approach? Also, I limited the scope to functions only, following the issue description, but if you want we can extend it to properties as well, need to double-check.
There was a problem hiding this comment.
I think when I did the initial implementation, I tried to add visibility modifiers to implicitModifiers but that didn't work for PUBLIC, since it undergoes a contrived logic I couldn't fully grasp and didn't feel confident modifying that, so I turned to this approach.
There was a problem hiding this comment.
@Egorand so what should we do with this implementation, do you have any advice?
There was a problem hiding this comment.
I think the least amount of changes should not be the priority here, and your PR IMO adds complexity rather than reducing it. I believe implicitModifiers is the right tool for the job, and it'd be great to remove the obstacles preventing it from working correctly - e.g. the custom handling of PUBLIC should probably be removed in favour of achieving the same result with implicitModifiers. This will likely require deeper knowledge of the library's inner workings and understand if you don't have the time to invest, but I don't think current approach is the best one.
There was a problem hiding this comment.
Ok, thanks for your feedback. You're right, this solution increases complexity and it was a quick workaround for implicitModifiers not working as expected. I'll try to dedicate some time to this issue and rework the solution following your guidelines.
When generating the string representation of functions inside a private type, all non-private modifiers (PUBLIC, INTERNAL) are omitted.
Fixes #1301