-
Notifications
You must be signed in to change notification settings - Fork 41
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
Inherit protocol modifiers #80
Conversation
These should be in the spy factory tests.
Hi @bradleymackey, welcome to the project! Thank you for your contribution! I'll try to review it over the weekend! 🙌 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
+ Coverage 96.69% 96.92% +0.23%
==========================================
Files 17 18 +1
Lines 695 748 +53
==========================================
+ Hits 672 725 +53
Misses 23 23 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, I want to apologize for the delay in providing feedback on your PR. Your effort and dedication are truly appreciated.
Upon reviewing the code, an alternative implementation occurred to me. I suggest considering the use of SyntaxRewriter. This would allow us to traverse the spy's final declaration, adjusting each DeclModifierListSyntax
to the required access level.
This method could streamline our code by eliminating the need to extend each method in every factory with an additional modifiers
parameter. The result would likely be a cleaner and more concise codebase.
I'm eager to hear your thoughts on this suggestion. I understand it might be a bit disheartening to revise what you've already accomplished so well, but I believe the end product will justify the effort.
Should you choose to adopt this approach, be aware of certain edge cases, like ensuring DeclModifierListSyntax
is not modified when its parent node is FunctionParameterSyntax
, as FunctionParameterSyntax
cannot have an access level modifier.
Here's an example of how you could incorporate such a syntax rewriter in SpyableMacor.swift
:
var spyClassDeclaration = try spyFactory.classDeclaration(for: protocolDeclaration)
// Considerations: Should extractor.extractAccessLevel return nil? Are we supporting all possible modifiers?
if let accessLevel = extractor.extractAccessLevel(from: protocolDeclaration) {
let accessLevelModifierRewriter = AccessLevelModifierRewriter(newAccessLevel: accessLevel)
spyClassDeclaration = accessLevelModifierRewriter.rewrite(spyClassDeclaration).cast(ClassDeclSyntax.self)
}
Looking forward to your input and next steps!
Hey @Matejkob could you merge this improvement by any chance? |
Oops, this one fell off my radar, apologies for not addressing the feedback as requested. I'm using Mockolo now for my personal projects as it has a lot of features I need anyway with a bit more control over certain aspects of the mock generation. I'll see about having a look at addressing these improvements in a week or so, but no promises as I may be a little busy. |
I would love to see this implemented because i am facing exactly the same issue. |
Hello! Do we have any updates on this? When will the @Spyable create a public class? |
Thank you so much @bradleymackey, for your hard work on this feature! Your approach of modifying each node individually was a great step forward and really helped shape the direction of this functionality. I’m closing this PR in favor of a new implementation (#130) that uses a syntax rewriter to handle access level inheritance. This simplifies the logic and ensures consistent behavior for all declaration members. Your contribution was invaluable, and I truly appreciate the effort you put into this. Looking forward to more contributions from you in the future! 😊 |
Hi @damian-kolasinski, @armintelker, @KonDouvris! Thank you for your patience and for highlighting the importance of this feature. I’ve created a new PR (#130 ) that implements access level inheritance using a syntax rewriter, ensuring consistent behavior across all members. I’m planning to merge it in 2/3 days to allow the community some time to review and provide feedback. I’d love to hear your thoughts or suggestions on the new implementation before it’s finalized! Your input is always appreciated. 😊 Thanks again for your support and enthusiasm for this project! |
public
,fileprivate
etc. to inherit this access on the spy and all the generated members. This is done by taking themodifiers
on the original protocol definition and applying it to all generated members.private
protocol members will inherit an access levelfileprivate
instead. Aprotocol
declaredprivate
at file-scope is effectively treated asfileprivate
. This access level on the generated members is required to satisfy the protocol requirementsinit
with the same access level, allowing the spy to be created outside of the current module (as the default init level isinternal
).Thanks for the great library! This is the best spy/mock generator out there atm.