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

Set up API client code generation #121

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

michpohl
Copy link
Contributor

@michpohl michpohl commented Oct 23, 2024

This PR adds the main code generation setup to :tidalapi, as well as the generated files.

Please review commit by commit to have an easy overview, and just skip the commit adding the generated files 😄

Note for reviewers:

The code in this pull request is a MVP. Please point out anything you don't like, but the idea is to get this one merged and then improve what needs improving in subsequent PRs. So if you request changes it should be for things that absolutely can't go in like this.
Please also look at the additional info provided in the commit messages.

Note about the generated code:

I debated whether it should be in the src folder or not, but after some reading I decided yes, because:

  • We will commit the generated code
  • Generation will not happen as part of the normal build process
  • Generated code in the future will be changed and reviewed via PR.

How can you test this?

  1. Check out the branch
  2. Publish to MavenLocal
  3. Add MavenLocal in settings.gradle.kts like this:
dependencyResolutionManagement {
    repositories {
        mavenCentral()
        }
}
  1. Add a dependency to com.tidal.sdk.tidalapi:0.1.0 where you want to use it, and instantiate an api client like this:
 val client: TidalApiClient = TidalApiClient(credentialsProvider)
  1. Play around with it.

Do you need to test it to review this?

At this stage, no imho. But if you like to, feel free!

How to/ test code generation?

As you currently need a binary of my fork of openapi-generator, ask me personally, if you want to play around with it.
But after this PR, we wil quickly move towards making this available via GH Actions, which will simplify this all.

@michpohl michpohl force-pushed the michael/Set-up-API-client-code-generation branch from b93ab79 to 57e785c Compare October 23, 2024 05:32
@michpohl michpohl force-pushed the michael/Set-up-API-client-code-generation branch from 57e785c to 265c7fa Compare October 23, 2024 11:17
@michpohl michpohl marked this pull request as ready for review October 23, 2024 11:17
@michpohl michpohl requested a review from a team as a code owner October 23, 2024 11:17
if result.returncode != 0:
logging.error("ktlint failed, but continuing with the rest of the script.")

target_line = "import com.tidal.sdk.tidalapi.generated.infrastructure.CollectionFormats.*"
Copy link
Contributor Author

@michpohl michpohl Oct 23, 2024

Choose a reason for hiding this comment

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

Note that ktlint cannot fix these imports (* imports of types provided by us), so after it is run, we manually remove them anywhere they show up as we don't need them, but openapi-generator just puts it everywhere


],
"output": "./tidal-api-joined.json",
"openapi_generator_path": "tbd"
Copy link
Contributor Author

@michpohl michpohl Oct 23, 2024

Choose a reason for hiding this comment

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

This would point to where the executable would be found. As I still work on providing it, it is tbd for now (which means, if you run the python script, it will download and merge the API schemas, but won't (re-) generate the code).

f"No lines starting with '{target_string}' found in file: {file_path}")


def merge_openapi_schemas(file_paths, output_path):

Choose a reason for hiding this comment

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

Why was it required for you? Would a simpler approach (just process all files one by one and overwrite models) like we did on iOS not work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I answered this already, but the answer got lost.
In short: I generate interfaces for each place a oneOf item is used - and those places are distributed across all APIS. So if I generate them consecutively, each generation overwrites code I just created. That's why the generator needs full context before run.

tidalapi/bin/generate-api-files.py Show resolved Hide resolved
@stoyicker
Copy link
Member

I don't see why we would want to commit the generated code. It sounds more reasonable to me to generate it on CI on publish, put it in the published binary and that's it.

@michpohl
Copy link
Contributor Author

michpohl commented Oct 24, 2024

I don't see why we would want to commit the generated code. It sounds more reasonable to me to generate it on CI on publish, put it in the published binary and that's it.

There are a number of reasons for it, here are some:

  1. We want engineers to easily review the changes in generated code before merging
  2. We want to keep open the ability to manually change details before merging, if needed
  3. API code generation (especially on Android, where we needed to alter the generator) is still quite brittle, and the schemas are still in a phase where they might change quite significantly. This approach makes it easier to step in and apply quick fixes

I know that you added a similar comment on my earlier PR, and I would have liked to do it that way too. I did a good amount of work towards that goal. But this approach made the most sense to thew devs involved (not just for Android) and so we decided to go with this way.

However, once we are past an MVP situation and both schema design and client use have stabilized it might be nice to revisit the approach and maybe reconsider/improve, but currently this is the way to go.

This adds a python script that does the following:
1. Download the schemas
2. Join the schemas (assuming all entries with the same name are equal, which we discussed they should be. This works as intended now).
3. Run openapi-generator-cli from a specified location (since we will have to submit our own fork).

Also added are the config yaml for the generator and the custom template for our ApiClient.
…yze mustache files

The pattern matching would catch .kt.mustache before, now it doesn't anymore.
openapi-generator has a poat-processiong option, but since we're using a dedicated script to run ktlint, I decided to add the step for this here.

With this change, ktlint will try and autoformat as many violations as possible. While not all of them get fixed, especially cleaning up imports is quite useful, as the generator creates a lot of unnecessary import statements.
This approach is taken from this tool:
https://www.npmjs.com/package/openapi-merge-cli
The tool itself didn't work out, but I like the approach over command line args, especially if its - like here - a number of long urls.
This ensures that in case of changed names/removals we don't end up with dead types that won't get removed.
@michpohl michpohl force-pushed the michael/Set-up-API-client-code-generation branch from 6d2c046 to 9573a62 Compare October 24, 2024 04:36
@michpohl michpohl added this pull request to the merge queue Oct 24, 2024
Merged via the queue into main with commit 1f65455 Oct 24, 2024
7 checks passed
@michpohl michpohl deleted the michael/Set-up-API-client-code-generation branch October 24, 2024 08:39
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.

4 participants