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.

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

Tidy up documentation scripts #5277

Merged
merged 10 commits into from
Jan 28, 2025
Merged

Tidy up documentation scripts #5277

merged 10 commits into from
Jan 28, 2025
+42 −43

Conversation

gonzaloriestra
Copy link
Contributor

WHY are these changes introduced?

We have too many similar scripts about documentation, all for different purposes, and it's really confusing:

  • docs:generate => API documentation
  • build-docs => shopify.dev static documentation
  • update-docs => shopify.dev commands + static documentation
  • refresh-documentation => autogenerated code documentation for ui.tsx

WHAT is this pull request doing?

Rename them so we have:

  • build-api-docs => API documentation
  • build-dev-docs => shopify.dev documentation
  • refresh-code-documentation => autogenerated code documentation for ui.tsx

How to test your changes?

Run them

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 27, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
75.36% (+0.07% 🔼)
 8933/11853
🟡 Branches
70.63% (+0.1% 🔼)
 4347/6155
🟡 Functions
75.14% (+0.09% 🔼)
 2346/3122
🟡 Lines
75.87% (+0.05% 🔼)
 8438/11121
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
 90.48% 98.61%
🟢
... / previewable-extension.ts
85.71% (-3.17% 🔻)
 100% 66.67%
83.33% (-4.17% 🔻)
🟢
... / setup-dev-processes.ts
95.56% (+0.1% 🔼)
78.57% (-3.57% 🔻)
 90.91%
94.87% (+0.13% 🔼)

Test suite run success

2011 tests passing in 905 suites.

Report generated by 🧪jest coverage report action from 6c6ef29

@gonzaloriestra gonzaloriestra marked this pull request as ready for review January 27, 2025 12:15
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner January 27, 2025 12:15
@gonzaloriestra Graphite App
Copy link
Contributor Author

gonzaloriestra commented Jan 27, 2025 •
edited
Loading

Merge activity

  • Jan 27, 7:47 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 27, 7:47 AM EST: A user added this pull request to the GitHub merge queue with Graphite.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Jan 27, 2025
@amcaplan amcaplan removed this pull request from the merge queue due to a manual request Jan 27, 2025
@amcaplan
Copy link
Contributor

I removed it from the queue because this will break the GitHub pages job, which uses docs:generate here and needs to be updated as well. No test would catch this.

@gonzaloriestra
Copy link
Contributor Author

@amcaplan the docs:generate target still exists for nx. I just updated the NPM script that it's not used there, no?

Btw, should we replace run: pnpm nx run-many --all --skip-nx-cache --target=docs:generate --output-style=stream with run: pnpm build-api-docs?

@amcaplan
Copy link
Contributor

Hmmmm, I am testing locally and @gonzaloriestra you're correct that nx is using project.json rather than package.json. But we still need the full command for the sake of --output-style=stream which is only appropriate for CI.

Also, I think that if we're renaming, we should be consistent throughout. So if we're calling this build-api-docs then it should be called that in cli-kit, across the project.json files, and in the GitHub action as well.

@gonzaloriestra
Squashed commit of the following:
5307cc1 
commit 7237405
Merge: 4a2ca0e 0ec203d
Author: Ariel Caplan <[email protected]>
Date:   Mon Jan 27 12:49:29 2025 +0000

    Merge pull request #5271 from Shopify/no-more-committed-docs-part-3

    Remove doc generation from the changeset-manifests workflow

commit 4a2ca0e
Merge: 6fba53f a4e532a
Author: Ariel Caplan <[email protected]>
Date:   Mon Jan 27 12:42:10 2025 +0000

    Merge pull request #5270 from Shopify/no-more-committed-docs-part-2

    Gitignore autogenerated docs

commit 6fba53f
Merge: ab1a33a e4f686a
Author: Shaun Stanworth <[email protected]>
Date:   Mon Jan 27 12:20:53 2025 +0000

    Merge pull request #5274 from Shopify/shauns/01-27-remotespecfication_includes_uidisclientprovided

    RemoteSpecfication includes uidIsClientProvided

commit ab1a33a
Merge: 8bd3b72 18783af
Author: Shaun Stanworth <[email protected]>
Date:   Mon Jan 27 12:14:17 2025 +0000

    Merge pull request #5273 from Shopify/shauns/01-27-app_management_schemas_are_found_on_the_main_branch

    App management schemas are found on the main branch

commit 8bd3b72
Merge: 33f6fb8 21e4ea4
Author: Ariel Caplan <[email protected]>
Date:   Mon Jan 27 12:09:50 2025 +0000

    Merge pull request #5263 from Shopify/no-more-committed-docs-part-1

    Add workflow to build a deploy artifact and deploy to GH pages

commit e4f686a
Author: Shaun Stanworth <[email protected]>
Date:   Mon Jan 27 11:09:22 2025 +0000

    RemoteSpecfication includes uidIsClientProvided

commit 18783af
Author: Shaun Stanworth <[email protected]>
Date:   Mon Jan 27 10:55:47 2025 +0000

    App management schemas are found on the main branch

commit 0ec203d
Author: Ariel Caplan <[email protected]>
Date:   Sun Jan 26 14:45:39 2025 +0200

    Remove doc generation from the changeset-manifests workflow

    We are using it when generating a released version, but it's no longer
    necessary if we are generating our docsite without committing the
    autogen pieces.

commit a4e532a
Author: Ariel Caplan <[email protected]>
Date:   Sun Jan 26 14:26:16 2025 +0200

    Remove tracking from docs/api

commit 21e4ea4
Author: Ariel Caplan <[email protected]>
Date:   Thu Jan 23 20:40:48 2025 +0200

    Add workflow to build a deploy artifact and deploy to GH pages
@gonzaloriestra
Rename docs:generate target to build-api-docs
b984153
@gonzaloriestra
Merge branch 'main' into rename-doc-scripts
9f224e6
@gonzaloriestra
Copy link
Contributor Author

@amcaplan I've checked that pnpm build-api-docs --output-style=stream works the same way.

And I've renamed the pending docs:generate stuff, I agree it's better now.

amcaplan
amcaplan approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@amcaplan amcaplan left a comment

Choose a reason for hiding this comment

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

Honestly including alphabetizing in this PR makes it very difficult to understand what exactly changed, but everything I can identify makes sense.

@gonzaloriestra Graphite App
Copy link
Contributor Author

I sorted it in a separate commit, so you can check the rest. But yeah, GH and Graphite could handle that much better...

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jan 28, 2025
@gonzaloriestra gonzaloriestra added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit 95b8209 Jan 28, 2025
26 checks passed
@gonzaloriestra gonzaloriestra deleted the rename-doc-scripts branch January 28, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amcaplan amcaplan amcaplan approved these changes

@isaacroldan isaacroldan isaacroldan approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@gonzaloriestra @amcaplan @isaacroldan