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 .shopify/metafields.json to .gitignore by default #5184

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

Conversation

karreiro
Copy link
Contributor

WHY are these changes introduced?

Fixes #5183

We've noticed a significant number of users discussing whether to ignore the .shopify/metafields.json file. While this decision ultimately rests with the developer, ignoring it by default seems like a sensible approach. This PR proposes implementing that default.

WHAT is this pull request doing?

This PR updates the .shopify/metafields.json creation to also add it to .gitignore.

How to test your changes?

  • Run shopify theme metafields pull
  • Notice that, when the .shopify/metafields.json file is created, it's also added in the gitingore

Post-release steps

N/A

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

@karreiro karreiro requested a review from aswamy January 13, 2025 10:43
@karreiro karreiro requested review from a team as code owners January 13, 2025 10:43
@karreiro karreiro changed the title When the .shopify/metafields.json file gets created, the CLI now proposes to add it to .gitignore by default Add .shopify/metafields.json to .gitignore by default Jan 13, 2025
@illarionvk
Copy link

It looks like your code always adds the line to .gitignore; how to opt-out of it?

Copy link
Contributor

github-actions bot commented Jan 13, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.14% (+0.02% 🔼)
8888/11829
🟡 Branches
70.29% (+0.02% 🔼)
4342/6177
🟡 Functions
75.07% (+0.03% 🔼)
2328/3101
🟡 Lines
75.69% (+0.02% 🔼)
8402/11100
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%

Test suite run success

2008 tests passing in 906 suites.

Report generated by 🧪jest coverage report action from bb40a59

@karreiro
Copy link
Contributor Author

Thank you for the review, @illarionvk!

It looks like your code always adds the line to .gitignore; how to opt-out of it?

It adds the line only when the file gets created. In subsequent synchronizations, it will no longer update the .gitignore. Users who prefer to sync the file can undo the .gitignore change and they won't be prompted to add that anymore.

Copy link
Contributor

@aswamy aswamy left a comment

Choose a reason for hiding this comment

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

❤️

packages/theme/src/cli/services/metafields-pull.test.ts Outdated Show resolved Hide resolved
packages/theme/src/cli/services/metafields-pull.ts Outdated Show resolved Hide resolved
@karreiro karreiro force-pushed the metafields-gitignore branch from 955ef22 to bb40a59 Compare January 14, 2025 18:04
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/fs.d.ts
@@ -278,6 +278,31 @@ export declare function glob(pattern: Pattern | Pattern[], options?: GlobOptions
  * @returns The File URL.
  */
 export declare function pathToFileURL(path: string): URL;
+/**
+ * The operating system-specific end-of-line marker:
+ * - 
+    node/20.18.1
+
+Use up/down arrow keys to select a version, return key to install, d to delete, q to quit on POSIX
+ * -  on Windows
+ */
+export type EOL = '
+' | '
+';
+/**
+ * Detects the end-of-line marker used in a string.
+ *
+ * @param content - file contents to analyze
+ *
+ * @returns The detected end-of-line marker
+ */
+export declare function detectEOL(content: string): EOL;
+/**
+ * Returns the operating system's end-of-line marker.
+ *
+ * @returns The OS-specific end-of-line marker
+ */
+export declare function defaultEOL(): EOL;
 /**
  * Find a file by walking parent directories.
  *

@karreiro karreiro added this pull request to the merge queue Jan 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025
Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

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

Thanks!

writeFileSync(filePath, fileContent)
}

function addToGitIgnore(root: string, entry: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Add .shopify/metafields.json to .gitignore by default
6 participants