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

Commit together with co-authors #1355

Merged
merged 69 commits into from Mar 26, 2018

Conversation

Projects
None yet
5 participants
@kuychaco
Member

kuychaco commented Mar 16, 2018

commit-with-friends

TODO:

  • update user store after initial loading (what if new commits are pulled with new authors?)
  • cache invalidation for getAuthors

In separate PRs:

  • use %x1F delimiter for getRecentCommits
  • hide behind feature flag
  • add new author (use prompt similar to CredentialDialog)
  • get username and avatar url from github graphql API
  • get rid of web committer (like so)
  • consider sorting based on most recent authors or # of commits rather than alphabetically
  • get rid of amend checkbox and implement undo and “amend” right-click instead
    • as is, clicking the amend checkbox will not strip co-author trailers and you could end up with duplicate co-authors. so we'll want to make this change before shipping to beta

/cc @niik @simurai

kuychaco and others added some commits Mar 21, 2018

Add authors cache key
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Add title to toggle co-authors icon for accessibility
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
🔥 login until we add github user info
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Don't call UserStore#populate in tests since it happens upon construc…
…tion

Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Add User Store test for refetching data when HEAD changes
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Fix test
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Trim message before committing and default coAuthors to empty array
(to fix tests)

Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
Add comment about caching
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
@simurai

How does the list get populated when searching for someone? Would it be possible to show username Real Name instead of Real Name email@address.com? Like GitHub Desktop.

Then it might also be possible to only show a person one time. Otherwise it's hard to decide which one to pick:

image

kuychaco added some commits Mar 22, 2018

@kuychaco kuychaco requested a review from smashwilson Mar 22, 2018

@kuychaco

This comment has been minimized.

Member

kuychaco commented Mar 22, 2018

@simurai great question! This PR just pulled author information from Git itself - from the local repository. In another PR we'll grab info from the graphql API and allow people to search via username. We decided on this approach first because we want to cover the base case where the user has a repo but doesn't necessarily have a github remote.

@smashwilson

Nice No blocking changes that I see. Merge whenever you're ready 😄

@@ -54,3 +54,16 @@
'.github-Dialog input':
'enter': 'core:confirm'
'body .github-CommitView-coAuthorEditor':

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Is body needed here for selector specificity reasons?

mentionableUsers: PropTypes.arrayOf(PropTypes.shape({
email: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
})).isRequired,

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

👍 for PropTypes.shape(). I know we've been a bit sloppy with our PropTypes sometimes 😄 Although it'll be a moot point if we do go to TypeScript 🤔

@@ -581,7 +581,7 @@ export default class GitShellOutStrategy {
// There's probably a better way. I tried finding a regex to do it in one fell swoop but had no luck
const coAuthors = body.split(LINE_ENDING_REGEX).reduce((emails, line) => {
const match = line.match(/\s*Co-authored-by: .*<(.*)>\s*/);
const match = line.match(/^co-authored-by: .*<(.*)>\s*/i);

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Good call 👍

return [];
} else {
throw err;
}

This comment has been minimized.

@smashwilson
for (const trailer of trailers) {
args.push('--trailer', `${trailer.token}=${trailer.value}`);
}
return this.exec(args, {stdin: commitMessage});

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Huh! TIL 🌠

if (!fakeEvent.defaultPrevented) {
e.abortKeyBinding();
}
};

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Huh, wacky 😄

'github:co-author:pagedown': this.proxyKeyCode(34),
'github:co-author:end': this.proxyKeyCode(35),
'github:co-author:home': this.proxyKeyCode(36),
'github:co-author:delete': this.proxyKeyCode(46),

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Random sidenote: we could move these into a <Commands> view for extra React Points.

@@ -47,7 +47,7 @@
"core-decorators": "^0.19.0",
"diff": "3.2.0",
"dompurify": "^1.0.3",
"dugite": "1.49.0",
"dugite": "^1.60.0",

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Ah good call.

@@ -179,5 +180,6 @@
"GitDockItem": "createDockItemStub",
"GithubDockItem": "createDockItemStub",
"FilePatchControllerStub": "createFilePatchControllerStub"
}
},
"false": {}

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

Hmm, what's this for?

await git.exec(['config', 'user.name', 'Com Mitter']);
await git.exec(['config', 'user.email', 'comitter@place.com']);
await git.exec(['commit', '--allow-empty', '--author="A U Thor <author@site.org>"', '-m', 'Commit together!']);

This comment has been minimized.

@smashwilson

smashwilson Mar 22, 2018

Member

🤣

@smashwilson

This comment has been minimized.

Member

smashwilson commented Mar 22, 2018

Huh. What's up with the failing tests, though? They look unrelated... ?

kuychaco and others added some commits Mar 22, 2018

@smashwilson smashwilson referenced this pull request Mar 24, 2018

Merged

Undo last commit and amend context menu option #1364

14 of 14 tasks complete
@kuychaco

This comment has been minimized.

Member

kuychaco commented Mar 26, 2018

For the sake of getting this feature out so that others can dog-food and provide input we're going to merge this as-is and implement all remaining to-dos in future PRs.

@kuychaco kuychaco merged commit b994822 into master Mar 26, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kuychaco kuychaco referenced this pull request Apr 3, 2018

Merged

Add dialog for new co-author #1374

25 of 33 tasks complete

@smashwilson smashwilson referenced this pull request Apr 19, 2018

Closed

Co-author commits #1309

@smashwilson smashwilson deleted the ku-niik/co-authors branch Sep 21, 2018

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