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 Vapor 4 client library #9583

Closed
wants to merge 5 commits into from

Conversation

aymanbagabas
Copy link
Contributor

@aymanbagabas aymanbagabas commented May 25, 2021

Opened in favor of #8515

TODOs:

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 force-pushed the swift5-vapor branch 2 times, most recently from 15e82ec to f8c9ec0 Compare May 26, 2021 13:48
@aymanbagabas
Copy link
Contributor Author

@4brunu CI is passing, Travis is failing cause of Docker.


{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}}API {
{{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}open{{/nonPublicApi}} class {{projectName}} {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aymanbagabas this is a breaking change that affects a lot of file, and what I remember from the other PR is that this change was only related to naming, so I suggest to revert it please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's only related to naming. However, it doesn't make sense to forcefully append "API" to the project name. The user should have a choice of having that in the name. Simply when desired, the user can append "API" to the project name and it would give the old behavior back.

I'll make a separate PR for that one. Should I base it on top of the 5.2.x branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I don't think this is an option worth to create.
It will add extra complexity and we won't add nothing new or extra functionality to the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aesthetics matter, having this suffix in the name is extra complexity. By merging it to the 5.2.x branch, we will introduce a breaking change with fallback and we don't have to introduce any flags or options since the fallback is to have API in the project name.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that would break every project that uses OpenAPI Swift Generator with compilation errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but for a generated library, I would rather have the project name matches the library class name since there is only one class. This also matches the SPM file structure for example the PlayingCard library

example-package-playingcard
├── Sources
│   └── PlayingCard
│       ├── PlayingCard.swift
│       ├── Rank.swift
│       └── Suit.swift
└── Package.swift

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point.
My main issue here is creating an extra burden on the user side.
To try to make this transition as smooth as possible, we could do something like this.

@available(*, deprecated, renamed: "PlayingCard")
public typealias PlayingCardAPI = PlayingCard

open class PlayingCard {
    ...
}

This way the old name PlayingCardAPI would still work, and the user could migrate when he wanted it.
Plus the IDE would presente a warning in a quick fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that a yes? Should I create a PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You last comment made sense to me, so for me it's a yes.
Could you please open a PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@4brunu I'm also thinking about doing a similar thing for Configuration.mustache and model.mustache when using the swiftUseApiNamespace option. I believe when users enable swiftUseApiNamespace, the Configuration class and models should also be under the API namespace like what api.mustache has. Because it doesn't make sense to do so for the API classes and not for the other classes. For example:

@available(*, deprecated, renamed: "PlayingCard.Configuration")
public typealias Configuration = PlayingCard.Configuration

extension PlayingCard {
    open class Configuration {
        ...
    }
}

and

@available(*, deprecated, renamed: "PlayingCard.Rank")
public typealias Rank = PlayingCard.Rank

extension PlayingCard {
    enum Rank: Int {}
}

@4brunu
Copy link
Contributor

4brunu commented May 27, 2021

@aymanbagabas I left a comment on an issue that is a breaking change and creates a lot of noise.
Could you please address it?
This PR contains a lot a changed files.
Ideally to make this PR easier to review and merge, this PR should only contain changes related to vapor that only affects the vapor clients.
Once this is addressed I will continue with the code review.

@aymanbagabas aymanbagabas mentioned this pull request May 30, 2021
5 tasks
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.

2 participants