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

feat: enable git diff on submodules #353

Merged
merged 5 commits into from
Sep 21, 2022

Conversation

hsaraujo
Copy link
Contributor

@hsaraujo hsaraujo commented Sep 18, 2022

Fixes isGit method to check whether .git is a folder or file (this is the case on submodules)

What does this pull request contain?


  • Added (for new features).
  • Changed (for changes in existing functionality).
  • Deprecated (for soon-to-be removed features).
  • Removed (for now removed features).
  • Fixed (for any bug fixes).
  • Security (for vulnerability fixes).

Explain your changes


It adds new condition on the isGit method that also checks whether a .git file exists

Where has this been tested?


Locally

Operating System: MacOS Monterey

yarn version: 1.22.19

node version: 18.8.0

git version: 2.37.0

sfdx version: 7.166.1

sgd plugin version:

Fixes isGit method to check whether .git is a folder or file (this is the case on submodules)
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (7e654b9) compared to base (a29baec).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #353   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines          733       735    +2     
=========================================
+ Hits           733       735    +2     
Impacted Files Coverage Δ
src/utils/cliHelper.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

Technically the new addition looks ok.
I think it could be improved with one or two unit test

// cliHelper.test.js
test('do not throw errors when repo contains git folder', async () => {
	...
})

test('do not throw errors when repo contains git file', async () => {
	...
})

@scolladon
Copy link
Owner

Hi @hsaraujo!

Thanks for this addition.
Could you educate us on which use case this PR is going to to enable?
I can think of one or two but I really would like your input here, maybe it would benefit from an explanation in the README.

@hsaraujo
Copy link
Contributor Author

Hi @scolladon thanks for the quick feedback.. I will add some unit tests to it.
This PR enables users to run the plugin on git projects that contains submodules. In these cases, the submodules don't have a .git folder on their root, but they do have a .git file.

Heitor Araujo added 2 commits September 21, 2022 11:27
fix fs mock for files in sub folders
adds coverage for git repo check on submodules
@hsaraujo
Copy link
Contributor Author

Hi @scolladon I've added some unit tests to it.
I also had to fix the fs mock implementation as it couldn't check subfolders either

scolladon
scolladon previously approved these changes Sep 21, 2022
Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM

Only a few remarks(nit) tag, up to you @hsaraujo to take them or not 😉
Let me know if you want to take care of them or not so I know if I should wait a bit before merging

__mocks__/fs.js Show resolved Hide resolved
__tests__/unit/lib/utils/cliHelper.test.js Outdated Show resolved Hide resolved
__tests__/unit/lib/utils/cliHelper.test.js Outdated Show resolved Hide resolved
__tests__/unit/lib/utils/cliHelper.test.js Outdated Show resolved Hide resolved
__tests__/unit/lib/utils/cliHelper.test.js Outdated Show resolved Hide resolved
__tests__/unit/lib/utils/cliHelper.test.js Show resolved Hide resolved
__tests__/unit/lib/utils/cliHelper.test.js Show resolved Hide resolved
src/utils/cliHelper.js Show resolved Hide resolved
removes unecessary data setup
@codeclimate
Copy link

codeclimate bot commented Sep 21, 2022

Code Climate has analyzed commit 7e654b9 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Owner

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for this nice contribution

@scolladon scolladon changed the title feat: enable git diff on submodules feat: enable git diff on submodules GA Sep 21, 2022
@scolladon scolladon changed the title feat: enable git diff on submodules GA feat: enable git diff on submodules Sep 21, 2022
@scolladon scolladon closed this Sep 21, 2022
@scolladon scolladon reopened this Sep 21, 2022
@scolladon scolladon merged commit 263e37d into scolladon:main Sep 21, 2022
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.

2 participants