refactor: simplify git helpers#134
Conversation
tjkandala
requested review from
a team,
felixfbecker and
marekweb
and removed request for
a team
| if (remote !== '') { | ||
| return gitRemoteURL(repoDirectory, remote) | ||
| async function gitRemoteNameAndBranch(repoDirectory: string): Promise<RemoteName & Branch> { | ||
| let remoteName = '' |
There was a problem hiding this comment.
Can we also change the pattern of using empty strings to using undefined? I.e. making a property optional if it can be optional, otherwise just relying on control flow analysis to make sure this is assigned.
Since a branch doesn't need to have an upstream I guess it would be optional? Or should the whole object be potentially undefined? I can't tell if this function name should read as "get the git remote name and get the branch name" or "get the git remote name and get the remote branch name"
There was a problem hiding this comment.
Right now we only use the branch if the remote is found, so we should return undefined instead of optional properties. This would also make our type simpler, since fileRelative is also dependent on the remote as well. We can revisit this function if it ever has more callers.
As for how to read the function name, I believe it's ideally the latter, but we fall back to 'HEAD' if we can't find the remote branch name.
| let remoteURL = '' | ||
| let branch = '' | ||
| let fileRelative = '' |
There was a problem hiding this comment.
It looks like we do this to "salvage" variables already re-assigned by the time an error is thrown. The issue is that whether we use the other values is dependent on whether remoteURL is found. Is there ever a case where we'd catch an error but still end up with remoteURL? I can't think of any since it's the last value we look for.
|
Let's not leave PRs hanging in open state for long - @marekweb could you review too, and @tjkandala are you planning on either making more changes or merging? |
|
@felixfbecker I made the changes you suggested, ready to merge now |
|
🎉 This PR is included in version 1.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
gitRemoteBranchwas called by bothgitDefaultRemoteURLandgitBranchand the return value was used differently in each call site