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] escape reserved words #9204

Merged

Conversation

aymanbagabas
Copy link
Contributor

@aymanbagabas aymanbagabas commented Apr 7, 2021

Swift reserved words should be escaped using backticks `` as the reference explains https://docs.swift.org/swift-book/ReferenceManual/LexicalStructure.html#ID412

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.1.x, 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.

@aymanbagabas aymanbagabas changed the title Swift5 escape reserved words [Swift5] escape reserved words Apr 7, 2021
@aymanbagabas aymanbagabas force-pushed the swift5-escape-reserved-words branch 3 times, most recently from e72b19b to a8f8e26 Compare April 7, 2021 17:03
@4brunu
Copy link
Contributor

4brunu commented Apr 7, 2021

This is a nice improvement, but this is a breaking change, so we need to create a flag that keep's the old behaviour by default.

@aymanbagabas
Copy link
Contributor Author

This is a nice improvement, but this is a breaking change, so we need to create a flag that keep's the old behaviour by default.

I think this should be the default behavior but I understand that this would introduce a breaking change. Thus, I think it's best to merge this to the 5.2.x branch instead of master.

@aymanbagabas aymanbagabas changed the base branch from master to 5.2.x April 7, 2021 18:22
@4brunu
Copy link
Contributor

4brunu commented Apr 7, 2021

For this to be merged in the branch 5.2.x it new to have a fallback to the old behaviour.
To do what you suggest, we need to merge this to 6.x.x.
Check compatibility in the readme for more info.
https://github.com/OpenAPITools/openapi-generator#11---compatibility

@aymanbagabas aymanbagabas changed the base branch from 5.2.x to 6.0.x April 7, 2021 18:34
@aymanbagabas
Copy link
Contributor Author

For this to be merged in the branch 5.2.x it new to have a fallback to the old behaviour.
To do what you suggest, we need to merge this to 6.x.x.
Check compatibility in the readme for more info.
https://github.com/OpenAPITools/openapi-generator#11---compatibility

Will the changes from the other PRs be merged to 6.0.x if they get merged to master?

@4brunu
Copy link
Contributor

4brunu commented Apr 7, 2021

Yes, but the version 6.0.x will only be release to the end of the year, or begin of next year.
So we should only consider that as last resort in case we can't find a way to make this backwards compatible with a switch.
But since this is a small change, we can create a switch to turn this on or off.

@aymanbagabas aymanbagabas changed the base branch from 6.0.x to master April 7, 2021 19:14
@aymanbagabas aymanbagabas force-pushed the swift5-escape-reserved-words branch 4 times, most recently from 57948f2 to a9e2528 Compare April 8, 2021 03:07
@4brunu
Copy link
Contributor

4brunu commented Apr 8, 2021

@aymanbagabas thanks for the changes.
Looks good to me.
The failure in CI is not related to the PR.
@wing328 for me this PR is ready to merge 👍

@wing328
Copy link
Member

wing328 commented Apr 21, 2021

@aymanbagabas please resolve the merge conflicts when you've time.

@aymanbagabas
Copy link
Contributor Author

@aymanbagabas please resolve the merge conflicts when you've time.

Resolved

@wing328 wing328 merged commit 33107c1 into OpenAPITools:master Apr 22, 2021
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

3 participants