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

Remove dependency on current-git-branch #335

Merged
merged 2 commits into from
Mar 4, 2024
Merged

Conversation

askoufis
Copy link
Contributor

@askoufis askoufis commented Mar 2, 2024

Replaces current-git-branch, a relatively simple, yet likely unmaintained package, with a call to git branch --show-current.

@askoufis askoufis requested a review from a team as a code owner March 2, 2024 09:50
Copy link

changeset-bot bot commented Mar 2, 2024

🦋 Changeset detected

Latest commit: 8dc4435

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
playroom Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

*/
const getGitBranch = () => {
try {
return execSync('git branch --show-current').toString().trim();
Copy link
Member

Choose a reason for hiding this comment

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

I've previously been burnt by OS differences when running raw child_process commands.

Is this something we should worry about?

I believe current-git-branch was using execa which builds in some "Make work on Windows" stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think all of the stuff execa (via cross-spawn) solves as far as Windows is concerned is irrelevant for this very simple command. I've just tested this on my Windows machine in both Cmd and PowerShell, and it worked fine.

Copy link
Member

@jahredhope jahredhope left a comment

Choose a reason for hiding this comment

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

Love the removal of the dep, and since it was tested in Windows seems good to me.

@askoufis askoufis enabled auto-merge (squash) March 4, 2024 05:11
@askoufis askoufis merged commit cad1ded into master Mar 4, 2024
6 checks passed
@askoufis askoufis deleted the remove-current-git-branch branch March 4, 2024 05:16
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.

None yet

2 participants