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

Support optional path parameters #56

Closed

Conversation

jordanekay
Copy link
Contributor

@jordanekay jordanekay commented May 15, 2024

See example here.

Copy link
Owner

@joshuawright11 joshuawright11 left a comment

Choose a reason for hiding this comment

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

This is great @jordanekay -

A quick piece of feedback and then this is ready to merge.

Thanks for the contribution - a solid improvement to path parameters 👍

Comment on lines +91 to +110
\(declaration) pathComponents: [String] = [\(
pathComponents
.filter { !$0.isEmpty }
.map { "\"\($0)\"" }
.joined(separator: ", "))]
"""

for (index, component) in pathComponents.filter({ !$0.isEmpty }).enumerated() {
if let parameter = pathParameter(from: component) {
buildRequest += """
if \(parameter) as Any? == nil {
pathComponents.remove(at: \(index))
}
"""
}
}

buildRequest += """
let path = pathComponents.joined(separator: "/")
var req = builder(method: \"\(method)\", path: path)
Copy link
Owner

Choose a reason for hiding this comment

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

Can we move this logic out of the @API macro and into the RequestBuilder?

I've been trying to keep all the "request construction" logic inside the RequestBuilder so that the generated code is dead simple to read.

Can make RequestBuilder.parameters a [String: String?] and then in the parameterizedPath() function exclude parameters that have nil values.

@joshuawright11
Copy link
Owner

@jordanekay did you want to merge this PR after the feedback? Otherwise I've got some time this weekend an can get this functionality merged in 😄

@jordanekay
Copy link
Contributor Author

Let’s close this—I’ll let you take it from here. Thank you!

@joshuawright11
Copy link
Owner

Cool - also FWIW the Alamofire dependency is dropped on main as well; will cut a release with that and the optional path parameters soon.

Please do let me know if there's any more features needed / blockers for using this library for parts of your project!

@jordanekay jordanekay closed this Jun 12, 2024
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