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

[New] Add --remote arg #31

Merged
merged 2 commits into from
Dec 3, 2021
Merged

[New] Add --remote arg #31

merged 2 commits into from
Dec 3, 2021

Conversation

Green-Ranger11
Copy link
Collaborator

@Green-Ranger11 Green-Ranger11 commented Nov 18, 2021

Adds a remote flag to specify which remote the user wishes to choose from.

Also fixes issue where if can-merge is run outside of a git repo it will throw error.

This closes #18

utils/isGitRepo.js Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
@Green-Ranger11 Green-Ranger11 changed the title [New] Add --remote flag [New] Add --remote arg Nov 18, 2021
bin/can-merge Show resolved Hide resolved
utils/getSHA.js Outdated Show resolved Hide resolved
utils/getRepo.js Outdated
return getRepos.match(pushRepoRegex)[0];
module.exports = function getRepo(remote = '') {
if (isGitRepo().sha) {
const getRepos = String(execSync(`git ls-remote --get-url ${remote}`));
Copy link
Owner

Choose a reason for hiding this comment

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

Is this data perhaps on the isGitRepo() object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately this is the one drawback since it doesn't have repo name but seemingly has everything else. The isGitRepo().sha is what I am using to check if its a git repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the only properties it has:

info.branch               // current branch
info.sha                  // current sha
info.abbreviatedSha       // first 10 chars of the current sha
info.tag                  // tag for the current sha (or `null` if no tag exists)
info.lastTag              // tag for the closest tagged ancestor
                          //   (or `null` if no ancestor is tagged)
info.commitsSinceLastTag  // number of commits since the closest tagged ancestor
                          //   (`0` if this commit is tagged, or `Infinity` if no ancestor is tagged)
info.committer            // committer for the current sha
info.committerDate        // commit date for the current sha
info.author               // author for the current sha
info.authorDate           // authored date for the current sha
info.commitMessage        // commit message for the current sha
info.root                 // root directory for the Git repo or submodule
                          //   (if in a worktree, this is the directory containing the original copy)
info.commonGitDir         // directory containing Git metadata for this repo or submodule
                          //   (if in a worktree, this is the primary Git directory for the repo)
info.worktreeGitDir       // if in a worktree, the directory containing Git metadata specific to
                          //   this worktree; otherwise, this is the same as `commonGitDir`. 

Copy link
Owner

Choose a reason for hiding this comment

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

gotcha. maybe we could make a PR to the package to add that info?

utils/getSHA.js Outdated Show resolved Hide resolved
utils/getRepo.js Outdated Show resolved Hide resolved
@Green-Ranger11 Green-Ranger11 linked an issue Nov 29, 2021 that may be closed by this pull request
bin/can-merge Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
utils/getRepo.js Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
bin/can-merge Outdated Show resolved Hide resolved
@Green-Ranger11 Green-Ranger11 force-pushed the remote-flag branch 2 times, most recently from eb1f926 to e15822e Compare December 2, 2021 22:35
@Green-Ranger11
Copy link
Collaborator Author

As discussed in our meeting such as adding tests for getSha should those go into a separate PR or should it be added to this?

@ljharb
Copy link
Owner

ljharb commented Dec 2, 2021

It'd be ideal to add them here, but it's also totally fine to do it separately. Your preference.

@ljharb
Copy link
Owner

ljharb commented Dec 2, 2021

Actually let me land this now, so we can rebase the other one.

@Green-Ranger11
Copy link
Collaborator Author

No worries at all! When this lands I'll open another one to add the test

@ljharb
Copy link
Owner

ljharb commented Dec 3, 2021

I ended up refactoring a bit, using the "boxed primitive object" trick, and ended up doing two .checks and a non-global .middleware.

{
"files": "bin/**",
"rules": {
"no-throw-literal": "off",
Copy link
Owner

Choose a reason for hiding this comment

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

this is because throwing a string literal skips an unnecessary stack trace :-)

@ljharb ljharb merged commit b0e2faa into main Dec 3, 2021
@ljharb ljharb deleted the remote-flag branch December 3, 2021 06:37
@Green-Ranger11 Green-Ranger11 linked an issue Jan 27, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants