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

Add SDK Typedoc Implementation for v3 Client #701

Merged
merged 13 commits into from
May 14, 2024

Conversation

ajimae
Copy link
Contributor

@ajimae ajimae commented May 9, 2024

Summary

Add SDK Typedoc Implementation for v3 Client

Completed Tasks

  • restructure configurations to for monorepo/workspace mode
  • add individual tsconfig.json and typedoc.json config files
  • add base tsconfig.base.json and typedoc.base.json to hold common configurations
  • specify namespaces for each module or packages
  • update github actions workflow file to script Version Package PRs

- restructure configurations to for monorepo/workspace mode
- add individual tsconfig.json and typedoc.json config files
- add base tsconfig.base.json and typedoc.base.json to hold common configurations
- specify namespaces for each module or packages
- update github actions workflow file to script Version Package PRs
@ajimae ajimae requested a review from a team as a code owner May 9, 2024 00:14
Copy link

changeset-bot bot commented May 9, 2024

⚠️ No Changeset found

Latest commit: 4140086

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

ajimae added 2 commits May 9, 2024 02:25
- remove the files configuration
- remove the files configuration
Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.83%. Comparing base (30d3ac5) to head (4140086).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #701   +/-   ##
=======================================
  Coverage   92.83%   92.83%           
=======================================
  Files          25       25           
  Lines         279      279           
  Branches       12       12           
=======================================
  Hits          259      259           
  Misses         20       20           
Flag Coverage Δ
integrationtests 92.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,15 +1,14 @@
name: SDK Type Docs Specification
on:
push:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why you removed this part? Will it now be deployed on every 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.

This was remove because of double deployment, as it will be deployed both for all PRs and also when the PR is merged which I don't think is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it will be available in GH pages? How will it ensure that PR won‘t overwrite the current docs on master if there is only 1 URL for the GH pages?

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 am not sure I understand, but how it works is that any changes made through a PR will build a new version of the docs and deploy it to gh-pages. This is to ensure that whatever modification is made to the SDK will automatically be updated in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also, there is no docs in the master, all the docs are built and deployed to gh-pages during PR builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then if the docs changes in PR is deployed to the gh-pages, the docs will be available to the users, but they will not see the correct classes in the sdk because the PR is not merged to master?

This doesn't matter, it is not as if the PR will take forever before it is merged. I think the time between raising the PR and getting it merged is relatively short to worry about this scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well that is not how the official docs work, if we have to look at it this way, the javadoc doesn't show any deployment in the PR to see how the docs looks like, I don't think we should go with that approach.

The official docs gives you a preview link which can be inspected to see the changes before the PR is merged, I think this is a better approach than the javadocs implementation.

I agree, so this GH pages link is for a preview only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well that is not how the official docs work, if we have to look at it this way, the javadoc doesn't show any deployment in the PR to see how the docs looks like, I don't think we should go with that approach.
The official docs gives you a preview link which can be inspected to see the changes before the PR is merged, I think this is a better approach than the javadocs implementation.

I agree, so this GH pages link is for a preview only?

No, it's not only for previews, it's also the live gh-pages link. I can change the trigger to merge or release only, but doing this, we will lose our ability to see the updated docs before merging the PR (no preview deployment link) which was something I thought about before going with the PR trigger approach.

But if you still think we should deploy it on merge or release, then we can talk to the team about it to see which approach is more favourable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let‘s discuss it in the tooling weekly 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mixed up the docs website with the ones for the SDK. The docs website publishes a preview for every PR using vercel. This is not the case for the SDK docs.

In the Java Repo we built the docs only for the main branch to see that it's not broken, but there is no deployment happening.

Deploy to the the GH pages happens only after a tag has been made or the build was triggered manually. This should be sufficient as the docs are mainly auto generated. Only manually written docs are sometimes built locally to check the look.

tsconfig.base.json Outdated Show resolved Hide resolved
Co-authored-by: Jens Schulze <[email protected]>
ajimae and others added 2 commits May 13, 2024 13:38
- update doc.yml workflow file
- update workflows to use repository dispatch
.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@ajimae ajimae merged commit 787057c into master May 14, 2024
6 checks passed
@ajimae ajimae deleted the feat/implement-complete-sdk-specs-v3 branch May 14, 2024 13:05
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.

3 participants