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 --strict flag to theme push to enforce theme check before pushing #4900

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Nov 21, 2024

WHY are these changes introduced?

Adds a new --strict flag to the theme push command that enforces theme check validation before allowing files to be pushed to a remote theme.

WHAT is this pull request doing?

Adds a new --strict flag to the theme push command that:

  • Runs theme check validation before pushing files
  • Blocks the push operation if any theme check errors are found (warnings are allowed)
  • Displays offense information and summary to help developers fix issues
  • Passes the output format down to theme check if JSON is specified
  • Adds tests

How to test your changes?

  1. Create a theme with some theme check violations
  2. Run theme push --strict
  3. Verify the command fails and displays theme check offenses
  4. Fix the violations
  5. Run theme push --strict again
  6. Verify the push succeeds with no violations

Errors

image.png

Warnings

image.png

Measuring impact

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

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

Copy link
Contributor

github-actions bot commented Nov 21, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
74.56% (-0.71% 🔻)
8554/11472
🟡 Branches
70.36% (-0.3% 🔻)
4173/5931
🟡 Functions
74.1% (-0.8% 🔻)
2246/3031
🟡 Lines
75.11% (-0.72% 🔻)
8090/10771
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / bundler.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app.test-data.ts
91.44% (-1.07% 🔻)
93.4%
81.25% (-2.5% 🔻)
90.86% (-1.14% 🔻)
🟢
... / release.ts
93.33%
58.33% (-11.67% 🔻)
75% 92.86%
🟢
... / app-event-watcher.ts
90.67% (-4.07% 🔻)
80% (-10.91% 🔻)
90.48%
95.52% (-3.01% 🔻)
🟢
... / app-watcher-esbuild.ts
83.33% (-5.56% 🔻)
87.5%
80% (-6.67% 🔻)
87.88% (-6.06% 🔻)
🔴
... / dev-session.ts
4.21% (-81.75% 🔻)
0% (-68.33% 🔻)
4.35% (-87.32% 🔻)
4.76% (-88.17% 🔻)
🟢
... / Replay.tsx
91.67% (-0.33% 🔻)
69.23% 100%
91.3% (-0.36% 🔻)
🔴
... / app-management-client.ts
20.33% (-4.99% 🔻)
9.52% (-3.73% 🔻)
22.11% (-4.49% 🔻)
18.75% (-5.23% 🔻)

Test suite run success

1949 tests passing in 886 suites.

Report generated by 🧪jest coverage report action from c94de67

Copy link
Contributor Author

jamesmengo commented Nov 21, 2024

@jamesmengo jamesmengo marked this pull request as ready for review November 21, 2024 20:10
@jamesmengo jamesmengo requested review from a team as code owners November 21, 2024 20:10

This comment has been minimized.

@jamesmengo jamesmengo self-assigned this Nov 21, 2024
@jamesmengo jamesmengo added the Area: @shopify/theme @shopify/theme package issues label Nov 21, 2024 — with Graphite App
Copy link
Contributor

@EvilGenius13 EvilGenius13 left a comment

Choose a reason for hiding this comment

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

🚀 Looks good!

@jamesmengo jamesmengo changed the base branch from lukeh/theme-pull-git-dir to main November 25, 2024 18:46
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/git.d.ts
@@ -97,6 +97,13 @@ export declare class OutsideGitDirectoryError extends AbortError {
  * @param directory - The directory to check.
  */
 export declare function ensureInsideGitDirectory(directory?: string): Promise<void>;
+/**
+ * Returns true if the given directory is inside a .git directory tree.
+ *
+ * @param directory - The directory to check.
+ * @returns True if the directory is inside a .git directory tree.
+ */
+export declare function insideGitDirectory(directory?: string): Promise<boolean>;
 export declare class GitDirectoryNotCleanError extends AbortError {
 }
 /**

Changeset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: @shopify/theme @shopify/theme package issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants