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 leading spacing on property and function declaration expansion #49

Merged
merged 1 commit into from
Dec 31, 2023

Conversation

dafurman
Copy link
Contributor

@dafurman dafurman commented Oct 28, 2023

Context
Indentation and spacing was a little off in some Macro expansion, so I wanted to make it more readable.

Given the example protocol:

@Spyable
public protocol ServiceProtocol {
    var name: String {
        get
    }
    var secondName: String? {
        get
    }

    func initialize(name: String, secondName: String?)
}

This is the expansion in main:

class ServiceProtocolSpy: ServiceProtocol {
    var name: String {
        get {
            underlyingName
        }
        set {
            underlyingName = newValue
        }
    }
    var underlyingName: (String )! ⬅️❌
        var secondName: String? ⬅️❌
    var initializeNameSecondNameCallsCount = 0
    var initializeNameSecondNameCalled: Bool {
        return initializeNameSecondNameCallsCount > 0
    }
    var initializeNameSecondNameReceivedArguments: (name: String, secondName: String?)?
    var initializeNameSecondNameReceivedInvocations: [(name: String, secondName: String?)] = []
    var initializeNameSecondNameClosure: ((String, String?) -> Void)?

        func initialize(name: String, secondName: String?) { ⬅️❌
        initializeNameSecondNameCallsCount += 1
        initializeNameSecondNameReceivedArguments = (name, secondName)
        initializeNameSecondNameReceivedInvocations.append((name, secondName))
        initializeNameSecondNameClosure?(name, secondName)
    }
}

And this is the expansion in this PR:

class ServiceProtocolSpy: ServiceProtocol {
    var name: String {
        get {
            underlyingName
        }
        set {
            underlyingName = newValue
        }
    }
    var underlyingName: (String)! ⬅️✅
    var secondName: String? ⬅️✅
    var initializeNameSecondNameCallsCount = 0
    var initializeNameSecondNameCalled: Bool {
        return initializeNameSecondNameCallsCount > 0
    }
    var initializeNameSecondNameReceivedArguments: (name: String, secondName: String?)?
    var initializeNameSecondNameReceivedInvocations: [(name: String, secondName: String?)] = []
    var initializeNameSecondNameClosure: ((String, String?) -> Void)?

    func initialize(name: String, secondName: String?) { ⬅️✅
        initializeNameSecondNameCallsCount += 1
        initializeNameSecondNameReceivedArguments = (name, secondName)
        initializeNameSecondNameReceivedInvocations.append((name, secondName))
        initializeNameSecondNameClosure?(name, secondName)
    }
}

Testing
Existing tests pass, and the adjusted tests pass.
Screenshot 2023-10-28 at 2 02 57 PM

@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bb042da) 96.43% compared to head (999e2f8) 96.52%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   96.43%   96.52%   +0.08%     
==========================================
  Files          16       16              
  Lines         646      662      +16     
==========================================
+ Hits          623      639      +16     
  Misses         23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Matejkob
Copy link
Owner

Hello @dafurman, thank you once again for your valuable contribution! I'm currently refactoring the factories in our project to enhance the capabilities of SwiftSyntaxBuilder. I have some concerns about merging your changes prior to this refactor. Would it be alright if we hold off on merging for a bit?

@dafurman
Copy link
Contributor Author

Hello @dafurman, thank you once again for your valuable contribution! I'm currently refactoring the factories in our project to enhance the capabilities of SwiftSyntaxBuilder. I have some concerns about merging your changes prior to this refactor. Would it be alright if we hold off on merging for a bit?

Yeah absolutely! No rush on this at all! I don't mind handling the git conflicts and reworking this if necessary after your refactor.

@Matejkob
Copy link
Owner

I've modified several files and simplified the implementation of some nodes, which has resulted in a few conflicts in this PR. Will you find time to resolve them? If not, I can attempt to address them in my free time.

@dafurman
Copy link
Contributor Author

Will you find time to resolve them? If not, I can attempt to address them in my free time.

Sure! I can probably get to that later today or this weekend!

@dafurman
Copy link
Contributor Author

@Matejkob The PR should be good to go for review now!

Copy link
Owner

@Matejkob Matejkob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a great improvement! I was quite annoyed about these leading spaces... Great work! Thanks for doin' it! 🫶

@Matejkob Matejkob merged commit 9491fc7 into Matejkob:main Dec 31, 2023
4 checks passed
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.

None yet

2 participants