Include deletions in modified files mask

[jakeleveroni]

Problem:

The git.getModifiedFiles method does not include removed files.

Solution:

The diff filter being used does not include the deletions (D) mask. Changing the diff-filter from ACMR to ACMRD includes deleted files in the results of the git.getModifiedFiles method.

Related issues:

Fixes #706

Checklist:

• Added or updated tests
  • unsure of how to test this, suggestions are appreciated!
• Added or updated documentation
• Ensured the pre-commit hooks ran successfully

By opening this pull request, you agree that this submission can be released under the same License that covers the project.

[netlify]

✅ Deploy Preview for onerepo canceled.

Name	Link
🔨 Latest commit	a13484a
🔍 Latest deploy log	https://app.netlify.com/sites/onerepo/deploys/66512c185761630008e4756b

[paularmstrong]

As implemented, this will break any command that relies on getFilepaths and passes them through for operating on.

This is easy to see with eslint. In the oneRepo repository:

1. Delete prettier.config.cjs
2. Run one lint

one lint
 ┌ Filtering ignored files
 └ ✔ 43ms
 ┌ Lint files
 │ ERR Oops! Something went wrong! :(
 │ ERR
 │ ERR ESLint: 8.57.0
 │ ERR
 │ ERR No files matching the pattern "prettier.config.cjs" were found.
 │ ERR Please check for typing mistakes in the pattern.
 └ ✘ 1178ms
 ■ ✘ Completed with errors 1242ms

I don't think it's a good idea to change how getFilepaths works, as that would result in a breaking change here. So, in order to preserve that behavior, we should figure out which is best between:

1. Add a flag to getModifiedFiles to include deleted or not, eg getModifiedFiles({ includeDeleted: true })

2. OR – create a separate getModifiedFilesByStatus that returns an object of the shape:

   type ModifiedByStatus = {
     added: Array<string>;
     copied: Array<string>;
     modified: Array<string>;
     deleted: Array<string>;
     copied: Array<string>;
     updated: Array<string>;
   }

   This is likely a better long-term solution that would open more possibilities for other plugins and integrations, but may be slightly more work.

[jakeleveroni]

As implemented, this will break any command that relies on getFilepaths and passes them through for operating on.

This is easy to see with eslint. In the oneRepo repository:

1. Delete prettier.config.cjs
2. Run one lint

one lint
 ┌ Filtering ignored files
 └ ✔ 43ms
 ┌ Lint files
 │ ERR Oops! Something went wrong! :(
 │ ERR
 │ ERR ESLint: 8.57.0
 │ ERR
 │ ERR No files matching the pattern "prettier.config.cjs" were found.
 │ ERR Please check for typing mistakes in the pattern.
 └ ✘ 1178ms
 ■ ✘ Completed with errors 1242ms

I don't think it's a good idea to change how getFilepaths works, as that would result in a breaking change here. So, in order to preserve that behavior, we should figure out which is best between:

1. Add a flag to getModifiedFiles to include deleted or not, eg getModifiedFiles({ includeDeleted: true })

2. OR – create a separate getModifiedFilesByStatus that returns an object of the shape:

   type ModifiedByStatus = {
     added: Array<string>;
     copied: Array<string>;
     modified: Array<string>;
     deleted: Array<string>;
     copied: Array<string>;
     updated: Array<string>;
   }

   This is likely a better long-term solution that would open more possibilities for other plugins and integrations, but may be slightly more work.

Thanks for the info, I also like the second option you mentioned. Ill try to get this updated this week or next week when i have some time. thanks for the feedback!

[jakeleveroni]

@paularmstrong I updated the original getModifiedFiles method to take an additional parameter to specify whether or not to include deletions. But i also implemented the getModifiedFileByStatus method cause it seemed like it might be useful at some point. I updated the tasks command to use the new method but it could just use the existing getModifiedFiles and pass true to the includeDeletions param. Not sure which way you prefer so lmk.

Additionally im not really sure how to write good tests around this since it requires git state mgmt, any pointers on how i can write meaningful tests for this?

GH Actions were down so i closed and opened the pr to kick them off again since they were stuck

[paularmstrong]

Nice. Almost there. I'm happy to take over on this if you don't have time.

[paularmstrong]

Additionally im not really sure how to write good tests around this since it requires git state mgmt, any pointers on how i can write meaningful tests for this?

@paularmstrong I updated the original getModifiedFiles method to take an additional parameter to specify whether or not to include deletions. But i also implemented the getModifiedFileByStatus method cause it seemed like it might be useful at some point. I updated the tasks command to use the new method but it could just use the existing getModifiedFiles and pass true to the includeDeletions param. Not sure which way you prefer so lmk.

I think what you've done here is good!

Additionally im not really sure how to write good tests around this since it requires git state mgmt, any pointers on how i can write meaningful tests for this?

I don't think I would write tests… there's a lot more setup necessary that's not really worth the payoff.

[jakeleveroni]

Nice. Almost there. I'm happy to take over on this if you don't have time.

Thanks for the offer! But I will have time tomorrow to wrap up those changes.

[paularmstrong]

@jakeleveroni I like what you did – and your changes helped me realize it could be simplified down to reuse the getModifiedFiles function just by including a single option. Take a look at the commit and let me know what you think.

[jakeleveroni]

@jakeleveroni I like what you did – and your changes helped me realize it could be simplified down to reuse the getModifiedFiles function just by including a single option. Take a look at the commit and let me know what you think.

Very nice, thats a clever implementation. I like it thanks for the assist! One thing im not quite understanding is the need for the template type ByStatusi see you use it to derive the reutrn type conditionally but does that mean you would need to call the method like so to get the modified by status response: getModifiedFiles<true>({ byStatus: true })?

[paularmstrong]

does that mean you would need to call the method like so to get the modified by status response: getModifiedFiles({ byStatus: true })?

Nope! It infers it internally. The getters for getFilepaths, for example, automatically knows the return type is an array of strings because byStatus is not set / false.

I need to dig a little bit more to figure out why I had to drop in the // @ts-expect-error though, so not quite done.