fix: fixed bug in old parser when using the defaults #200

WillieRuemmele · 2020-11-02T23:15:36Z

What does this PR do?

the default ignore lines we use with the new parser had a bug with the old parser. Specifically the **/*.dup* was being exploded to ignore any item containing "dup" which for the metadata type DuplicateRule caused an issue

fixed analytics to log the lines that caused the issue, and the file path

What issues does this PR fix or reference?

forcedotcom/cli#685
@W-8291520@

Functionality Before

Functionality After

shetzel · 2020-11-02T23:50:50Z

src/metadata-registry/forceIgnore.ts

@@ -81,7 +80,7 @@ export class ForceIgnore {
        denies = this.parser.ignores(relativePath);
        fctResult = this.gitignoreParser.denies(relativePath);
        // send to look for differences, analytics
-        this.resolveConflict(denies, fctResult, this.contents);
+        this.resolveConflict(denies, fctResult, this.contents, relativePath);


If it's an instance var you don't have to pass it to a method that has access to that instance var.

If we're passing the relativePath here then we don't need to do the relative path resolution in resolveConflict

shetzel · 2020-11-02T23:55:50Z

src/metadata-registry/forceIgnore.ts

@@ -150,7 +150,7 @@ export class ForceIgnore {
      // send analytics, if they exist.
      Lifecycle.getInstance().emit('telemetry', {
        eventName: 'FORCE_IGNORE_DIFFERENCE',
-        content: this.contents,
+        content,
        oldLibraryResults,
        newLibraryResults,
        ignoreLines: this.ignoreLines,


this.ignoreLines will always be the entire contents of the forceignore file. don't we want this to only report the specific rules that caused the conflict?

it was only the entries which caused issues... I renamed the variable to be more clear about what it is

shetzel · 2020-11-02T23:59:56Z

src/metadata-registry/forceIgnore.ts

@@ -122,6 +121,7 @@ export class ForceIgnore {
  private resolveConflict(
    newLibraryResults: boolean,
    oldLibraryResults: boolean,
+    content: string,
    file: string


newLibraryResults and oldLibraryResults are very clear. content and file aren't. The content arg doesn't seem necessary since it's always this.contents. Can we name those so it's clearer? E.g., forceignoreContents (if you keep it) and sourceFilePath?

brpowell · 2020-11-04T17:18:42Z

Revert these files please:

Code owners
.gitignore
snippets.code-snippets
CHANGELOG
Coding-guidelines
commit-guidelines

brpowell · 2020-11-04T17:23:47Z

src/metadata-registry/forceIgnore.ts

          // only show the warning once, it could come from denies() or accepts()
          warn = false;
          process.emitWarning(
-            "The .forceignore file doesn't adhere to .gitignore format which will be the default behavior starting in Spring '21 release. More information on .gitignore format here: https://git-scm.com/docs/gitignore. Fix the following lines in your .forceignore and add '# .forceignore v2' to your .forceignore file to switch to the new behavior."
+            "The .forceignore sourceFilePath doesn't adhere to .gitignore format which will be the default behavior starting in Spring '21 release. More information on .gitignore format here: https://git-scm.com/docs/gitignore. Fix the following lines in your .forceignore and add '# .forceignore v2' to your .forceignore sourceFilePath to switch to the new behavior."


should this say sourceFilePath?

yeah, I refactored a variable name and it didn't work correctly... look at all of the files I changed ( even though I didn't add them to my commit ) I'm going back through everything to make sure it's as it should be

brpowell · 2020-11-04T17:27:40Z

src/metadata-registry/forceIgnore.ts

  ): void {
    const ignoreItems = this.contents.split('\n');
+    const troubledIgnoreLines: string[] = [];


Use a Set<string> for troubledIgnoreLines and on line 136 use has instead of includes so we don't have to iterate through the whole list each time to test for membership.

brpowell · 2020-11-04T17:31:58Z

src/metadata-registry/forceIgnore.ts

@@ -122,29 +120,28 @@ export class ForceIgnore {
  private resolveConflict(
    newLibraryResults: boolean,
    oldLibraryResults: boolean,
-    file: string
+    sourceFilePath: string


nit: fsPath has somewhat been a codebase convention for "file path" if you're looking to rename this, nbd though.

brpowell · 2020-11-04T17:36:10Z

test/metadata-registry/forceIgnore.test.ts

-        '**/package2-descriptor.json',
-        '**/package2-manifest.json',
-      ],
+      ignoreLines: [testPattern],
      file,
    };

    const telemetrySpy = env.spy(Lifecycle.prototype, 'emit');
    // @ts-ignore call the private method directly to avoid excessive stubbing


Out of curiosity, what would need to be excessively stubbed?

At this point it might not require a lot of stubbing

If not much is needed then forceIgnore.accept or forceIgnore.deny is preferred over forceIgnore.resolveConflict(true, false, file)

brpowell · 2020-11-05T00:09:33Z

src/metadata-registry/forceIgnore.ts

@@ -153,8 +156,8 @@ export class ForceIgnore {
        content: this.contents,
        oldLibraryResults,
        newLibraryResults,
-        ignoreLines: this.ignoreLines,
-        file,
+        ignoreLines,


Can remove 147-151 and use this instead - easy way to convert an iterable to array.

Suggested change

ignoreLines,

ignoreLines: Array.from(troubledIgnoreLines),

changes were addressed and steve is out for the day.

* fix: The old parser had a bug when handling the new default values

fix: fixed bug in old parser when using the defaults

07a0daa

WillieRuemmele requested review from shetzel and brpowell November 2, 2020 23:15

WillieRuemmele requested a review from a team as a code owner November 2, 2020 23:15

shetzel previously requested changes Nov 3, 2020

View reviewed changes

fix: code review 1

3464e90

brpowell reviewed Nov 4, 2020

View reviewed changes

WillieRuemmele added 3 commits November 4, 2020 11:01

fix: reset unnecessarily changed files, changed to Set

055c0ad

fix: reset unnecessarily changed files

e0fb5da

fix: fixed unit tests that I forgot about

6a8a171

brpowell reviewed Nov 5, 2020

View reviewed changes

WillieRuemmele added 4 commits November 4, 2020 17:11

fix: use Array.from

b47e7a4

fix: update test to call accept directly

2629634

fix: update test to call accept directly

87771e4

fix: stub methods to force results

640dd25

brpowell approved these changes Nov 5, 2020

View reviewed changes

WillieRuemmele requested a review from shetzel November 9, 2020 17:37

brpowell removed the request for review from shetzel November 9, 2020 17:38

shetzel approved these changes Nov 9, 2020

View reviewed changes

WillieRuemmele merged commit 3ccbfa8 into develop Nov 9, 2020

WillieRuemmele deleted the wr/fixDefaultIgnore branch November 9, 2020 18:23

brpowell pushed a commit that referenced this pull request Nov 12, 2020

fix: fixed bug in old parser when using the defaults (#200)

26ea08e

* fix: The old parser had a bug when handling the new default values

brpowell pushed a commit that referenced this pull request Nov 12, 2020

fix: fixed bug in old parser when using the defaults (#200)

33c8f1d

* fix: The old parser had a bug when handling the new default values

sfsholden pushed a commit that referenced this pull request May 6, 2021

fix: fixed bug in old parser when using the defaults (#200)

4895609

* fix: The old parser had a bug when handling the new default values

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fixed bug in old parser when using the defaults #200

fix: fixed bug in old parser when using the defaults #200

WillieRuemmele commented Nov 2, 2020

shetzel Nov 2, 2020

shetzel Nov 2, 2020

WillieRuemmele Nov 4, 2020

shetzel Nov 2, 2020

brpowell commented Nov 4, 2020 •

edited

Loading

brpowell Nov 4, 2020

WillieRuemmele Nov 4, 2020

brpowell Nov 4, 2020

brpowell Nov 4, 2020

brpowell Nov 4, 2020

WillieRuemmele Nov 4, 2020

brpowell Nov 5, 2020

brpowell Nov 5, 2020 •

edited

Loading

WillieRuemmele Nov 5, 2020

fix: fixed bug in old parser when using the defaults #200

fix: fixed bug in old parser when using the defaults #200

Conversation

WillieRuemmele commented Nov 2, 2020

What does this PR do?

What issues does this PR fix or reference?

Functionality Before

Functionality After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brpowell commented Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brpowell Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brpowell commented Nov 4, 2020 •

edited

Loading

brpowell Nov 5, 2020 •

edited

Loading