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

[Swift5][client] try to fix JsonEncondable #11541

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

4brunu
Copy link
Contributor

@4brunu 4brunu commented Feb 7, 2022

Try to fix #11202 (comment)

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jgavris (2017/07) @ehyche (2017/08) @Edubits (2017/09) @jaz-ah (2017/09) @4brunu (2019/11)

@honkmaster
Copy link
Contributor

I'm away from my work computer at the moment, so I tried the approach in a playground. It seems to work. What I don't like however is the constant repetition. If you search the history, you will find the code almost exactly as you have implemented it now.

a620853

@4brunu
Copy link
Contributor Author

4brunu commented Feb 7, 2022

From my tests, the repetition is necessary to make this work.
If we rely on the extension's default implementation, it encodes the string as json.

@honkmaster
Copy link
Contributor

Yep. Agree. I don't mind the repetition if it works, I just think the resulting code is "unattractive".

@honkmaster
Copy link
Contributor

honkmaster commented Feb 7, 2022

An alternative approach would be to enhance the encodeToJSON in the protocol extension, e.g.:

extension JSONEncodable where Self: Encodable {
    func encodeToJSON() -> Any {
        if self is String || self is Double {
            return self
        }
        
        let encoder = JSONEncoder()
        guard let data = try? encoder.encode(self) else {
            fatalError("Could not encode to json: \(self)")
        }
        return data.encodeToJSON()
    }
}

But this would result in a rather long if statement in the beginning. Matter of taste.

@4brunu
Copy link
Contributor Author

4brunu commented Feb 8, 2022

Yes, that's another option.
I think I like more the current implementation in this PR, to avoid a big if else.
What about you?

@honkmaster
Copy link
Contributor

Yes, that's another option.
I think I like more the current implementation in this PR, to avoid a big if else.
What about you?

Again, Matter of taste. LGTM.

@4brunu 4brunu merged commit 441c069 into OpenAPITools:master Feb 9, 2022
@4brunu 4brunu deleted the fix/json-encondable-swift-v2 branch February 9, 2022 08:50
@4brunu
Copy link
Contributor Author

4brunu commented Feb 9, 2022

@honkmaster thanks for reporting this issue and for validating that it's working

@honkmaster
Copy link
Contributor

@4brunu Will this only be released in 6.0.0 or do you think there will be a 5.4.1?

@4brunu
Copy link
Contributor Author

4brunu commented Feb 21, 2022

According to the readme I think it will be available in 6.0.0.
https://github.com/OpenAPITools/openapi-generator#table-of-contents

@romanzhukovTB
Copy link

romanzhukovTB commented Mar 1, 2022

Hi @4brunu, can this fix be expedited? The project I'm working on is blocked by this bug. Or maybe if there's a suggested workaround - that would be good for now.

@4brunu
Copy link
Contributor Author

4brunu commented Mar 1, 2022

Hi @romanzhukovTB, while there isn't a new release, I suggest that you build the project locally and use openapi through the jar file.
https://github.com/OpenAPITools/openapi-generator#14---build-projects

@honkmaster
Copy link
Contributor

We reverted back to 5.3.1. Never upgrade a working system again ;).

@rhys-rant
Copy link

I am now also blocked by this issue, and would really appreciate either 6.0.0, or an expedited 5.4.1 if that's at all possible 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants