Skip to content

automatically sync API docs on rescript-lang.org #7555

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Jun 17, 2025
Merged

automatically sync API docs on rescript-lang.org #7555

merged 14 commits into from
Jun 17, 2025

Conversation

tsnobip
Copy link
Member

@tsnobip tsnobip commented Jun 16, 2025

No description provided.

@fhammerschmidt fhammerschmidt requested a review from cknitt June 16, 2025 21:54
@fhammerschmidt
Copy link
Member

@cknitt would be cool to have this merged before the next release so that it can be already testet fully.

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Jun 17, 2025 •
edited
Loading

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7555

@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7555

@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7555

@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7555

@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7555

@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7555

commit: 931c6f8

cknitt
cknitt reviewed Jun 17, 2025
View reviewed changes
package.json Outdated
@@ -52,7 +52,9 @@
"check:all": "biome check .",
"format": "biome check --changed --no-errors-on-unmatched . --fix",
"coverage": "nyc --timeout=3000 --reporter=html mocha tests/tests/src/*_test.js && open ./coverage/index.html",
"typecheck": "tsc"
"typecheck": "tsc",
"scripts:build": "rescript",
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? It's not any shorter than yarn rescript.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can get rid of it

Copy link
Member Author

Choose a reason for hiding this comment

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

actually that was a mistake, it should have been elsewhere, I'm removing it.

@fhammerschmidt
Copy link
Member

fhammerschmidt commented Jun 17, 2025 •
edited
Loading

Does it really need to execute on every PR or is that just for testing?
Because the sync step takes 2.45 minutes and only the commit and push part is omitted if it is not a tag build.

At least that part seems to work already :)

nojaf
nojaf reviewed Jun 17, 2025
View reviewed changes
@tsnobip
Copy link
Member Author

tsnobip commented Jun 17, 2025

Does it really need to execute on every PR or is that just for testing? Because the sync step takes 2.45 minutes and only the commit and push part is omitted if it is not a tag build.

At least that part seems to work already :)

@fham I think it's worth executing this on every PR in case of malformed docstrings. The cool thing is 2'26" taken by website build is only done when there are changes in docstrings! So the overhead is only a dozen seconds otherwise.

@fhammerschmidt
Copy link
Member

Ok I missed that part. I guess then it's totally fine.

@nojaf
Copy link
Collaborator

nojaf commented Jun 17, 2025

I noticed
image

on #7554

@tsnobip is this too be expected?

@tsnobip
Copy link
Member Author

tsnobip commented Jun 17, 2025

I noticed image

on #7554

@tsnobip is this too be expected?

@nojaf yeah I think so, we should only merge PRs that have valid docstrings, don't you think?

@nojaf
Copy link
Collaborator

nojaf commented Jun 17, 2025

@tsnobip got it! Was wondering if that did any actual syncing, which would be a little strange for a PR.

@tsnobip
Copy link
Member Author

tsnobip commented Jun 17, 2025

@tsnobip got it! Was wondering if that did any actual syncing, which would be a little strange for a PR.

@nojaf you're right, should I rename the job?

@nojaf
Copy link
Collaborator

nojaf commented Jun 17, 2025

should I rename the job?

Yes, please, that would be less confusing.

nojaf
nojaf approved these changes Jun 17, 2025
View reviewed changes
Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Ship it!

@tsnobip tsnobip merged commit 0ea2c00 into master Jun 17, 2025
17 checks passed
@tsnobip tsnobip deleted the gen-api-docs branch June 17, 2025 10:06
@tsnobip tsnobip mentioned this pull request Jun 17, 2025
Merged
tsnobip added a commit that referenced this pull request Jun 17, 2025
@tsnobip
fix api docs download path (#7560)
a76bba6 
* fix api docs download path from #7555

* removes extra newlines in docstrings

* add some logs when checking if website repo is clean

* format
@tsnobip tsnobip mentioned this pull request Jun 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojaf nojaf nojaf approved these changes

@fhammerschmidt fhammerschmidt fhammerschmidt approved these changes

@cknitt cknitt Awaiting requested review from cknitt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tsnobip @fhammerschmidt @nojaf @cknitt