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

Fix #1025 using git rev-parse to get the relative path inside git repo #1026

Merged
merged 2 commits into from Oct 9, 2021
Merged

Conversation

tkalmar
Copy link
Contributor

@tkalmar tkalmar commented Oct 7, 2021

Fix #1025
Idea for solution is from: pypa/setuptools_scm#415

@iiroj
Copy link
Member

iiroj commented Oct 7, 2021

Thanks for the PR!

Since this is a platform-specific fix, could you change this to an if clause with a code comment on why this is necessary?

Something like:

if (gitRel) {
  // <reason>
  return ...
} else {
  // reason why not
  return ...
}

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #1026 (e43880d) into master (32c08d3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1026   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          646       651    +5     
  Branches       146       148    +2     
=========================================
+ Hits           646       651    +5     
Impacted Files Coverage Δ
lib/resolveGitRepo.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32c08d3...e43880d. Read the comment docs.

Copy link
Member

@iiroj iiroj left a comment

Choose a reason for hiding this comment

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

Can you please edit the commit to include code comments explaining why this is necessary, according to this commit #1026 (comment)

This is only to make the code more readable and maintainable, thanks!

@tkalmar
Copy link
Contributor Author

tkalmar commented Oct 7, 2021

Done. Hope the intend is clearer now

Copy link
Member

@iiroj iiroj left a comment

Choose a reason for hiding this comment

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

I will merge this as fix: detect git repo root correctly on cygwin

@iiroj iiroj merged commit f291824 into lint-staged:master Oct 9, 2021
@iiroj
Copy link
Member

iiroj commented Oct 9, 2021

Thank you for your contribution, @tkalmar!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2021

🎉 This PR is included in version 11.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This PR is included in version 11.3.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Under cygwin no tasks are created
2 participants