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

requestOperationIdExtension adds private extension making it inaccessible if in a separate file #42

Closed
andrea-gradecak opened this issue Jun 15, 2022 · 11 comments · Fixed by #83
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@andrea-gradecak
Copy link

In Templates.swift, requestOperationIdExtension adds a private extension.

If generation creates separate files, the private extension is also in its own file Paths+Extensions which makes it inaccessible to use in other files.

@liamnichols liamnichols added the bug Something isn't working label Jun 15, 2022
@liamnichols liamnichols added the good first issue Good for newcomers label Jul 30, 2022
@liamnichols liamnichols added this to the 0.1.0 milestone Aug 2, 2022
@LePips
Copy link
Contributor

LePips commented Aug 2, 2022

Just taking a look at this, should we just always generate a functional helper:

public extension Request {
    public func settingID(_ id: String) -> Request { // different name, but functional in context
        var copy = self
        copy.id = id
        return copy
    }
}

@liamnichols
Copy link
Member

Could it even just be a case of removing the private from the existing template?

@LePips
Copy link
Contributor

LePips commented Aug 2, 2022

Ultimately yes, but this seems so small to be behind a flag especially since the user can just edit the id variable directly. Additionally, the functional name could be a change.

@LePips
Copy link
Contributor

LePips commented Aug 2, 2022

Or, this could be a change in Get itself as it seems more appropriate there

@liamnichols
Copy link
Member

Using isAddingOperationIds will fail if the schema doesn't specify an operationId for one or more of its operations, which is allowed as per the spec.

So at the moment, the flag is necessary. But that being said, I've seen other tools automatically infer the operationId from the path + method, which we could also do. And if we did, then we'd be able to just generate it automatically (or maybe just leave it as nil?)

Or, this could be a change in Get itself as it seems more appropriate there

Yeah I did think about that and it is also a possibility. It would simplify things

@LePips
Copy link
Contributor

LePips commented Aug 2, 2022

I think it would be safe to add the operationId as the Request.id by default, nil if none specified. Get does not use the id property so it's mainly for developer usage (unless I'm missing something or it has actual Request usage in somebody's package).

@liamnichols
Copy link
Member

Before I noticed the flag, I asked about the purpose in kean/Get#33 and you are right, it is designed for this.

So in that case, maybe we do just pass the id through the convenience methods.. something like this:

public static func get(_ url: String, query: [(String, String?)]? = nil, headers: [String: String]? = nil, id: String? = nil) -> Request {
    Request(method: "GET", url: url, query: query, headers: headers, id: id)
}

What do you think about this @kean?

@kean
Copy link
Member

kean commented Aug 2, 2022

I'm still debating how to design it. I don't think I've done a great job with all these telescoping methods. There are too long, duplicated, and I keep forgetting the order of parameters.

I'm thinking about taking a cue from the SwiftUI design:

// Now
_ = Request.post(
    "/domain.tld",
    query: [("query", "value1+value2")],
    body: value,
    headers: ["a": "b"]
).withId("getDomains")

// Target state
_ = Request.post("/domain.tld", body: value)
    .query([("query", "value1+value2")])
    .headers(["a": "b"])
    .id("getDomains")

But I'm not sure it'll work well with the existing writable properties with the same names. It does work, but I'm sure there are going to be some issues because of the name clashes.

@kean
Copy link
Member

kean commented Aug 2, 2022

Right now you can use the existing initializer for code generation purposes:

    public init(
        method: String = "GET",
        url: String,
        query: [(String, String?)]? = nil,
        body: Encodable? = nil,
        headers: [String: String]? = nil,
        id: String? = nil
    ) {

It didn't have the id parameter prior to Get 1.0, but it does now.

@liamnichols
Copy link
Member

Thanks for the input! Ok that makes sense, the telescoping methods (TIL) methods are more for manual usage and it doesn't really matter what the generated code uses. We can generate implementations that use Request.init(...) directly instead 👍

@kean
Copy link
Member

kean commented Aug 2, 2022

Yes, I recommend using the initializer because otherwise, you might end up using methods that'll get deprecated soon. The initializer won't change any time soon or ever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants