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

Improve mechanism that adds .shopify to .gitignore to avoid duplicate entries #5314

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

karreiro
Copy link
Contributor

WHY are these changes introduced?

Fixes #5286

WHAT is this pull request doing?

Instead of manipulating raw strings, now addToGitIgnore uses the ignore module (already being used by apps), so we may prevent equivalent patterns from being added.

It's important to notice though, that not all patterns reported on Fixes #5286 are equivalent as we may notice in the screenshot below:

image

How to test your changes?

  • Run shopify theme metafields pull
  • Remove .shopify/metafields.json
  • Remove any .shopify entry from .gitignore
  • Notice that, when the .shopify/metafields.json file is created, an entry is also added in the .gitignore

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 review from a team as code owners January 30, 2025 08:46
@karreiro karreiro requested a review from graygilmore January 30, 2025 08:47
Copy link
Contributor

github-actions bot commented Jan 30, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.49% (+0.01% 🔼)
9001/11924
🟡 Branches
70.67% (+0.01% 🔼)
4390/6212
🟡 Functions 75.29% 2362/3137
🟡 Lines
76% (+0.02% 🔼)
8503/11188
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%

Test suite run success

2034 tests passing in 909 suites.

Report generated by 🧪jest coverage report action from da606a6

Copy link
Contributor

@isaacroldan isaacroldan left a comment

Choose a reason for hiding this comment

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

Great idea using ignore 👌

const ignoreManager = ignore.default({allowRelativePaths: true}).add(lines)

const isIgnoredEntry = ignoreManager.ignores(joinPath(entry))
const isIgnoredEntryAsDir = ignoreManager.ignores(joinPath(entry, 'ignored.txt'))
Copy link
Contributor

Choose a reason for hiding this comment

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

What's ignored.txt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mocked file entry that supports the following two test scenarios:

- does nothing when .shopify/* pattern exists in .gitignore
- does nothing when .shopify/** pattern exists in .gitignore

The ignore package doesn't interact with the file system; it just analyzes paths and patterns. This mocked entry helps us ensure that .shopify/* and .shopify/** are really working as expected.

@karreiro
Copy link
Contributor Author

(I rebased the branch and fixed pnpm-lock.yaml)

@karreiro karreiro added this pull request to the merge queue Jan 31, 2025
Merged via the queue into main with commit c846e6f Jan 31, 2025
27 checks passed
@karreiro karreiro deleted the gitignore-bug branch January 31, 2025 11:00
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.

[Bug]: GitIgnore .shopify equivalents are ignored
3 participants