Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

@annthurium
Copy link

@annthurium annthurium commented Jan 12, 2019

Description of the Change

This change adds the committer's full name to the commit detail pane. Instead of email address, we show the full name.

screen shot 2019-01-15 at 1 38 23 pm

Alternate Designs

  • I considered showing the name and email address. However, it looked kinda cluttered, and I couldn't find a good way of showing co author info in that scenario.

image

Benefits

  • Makes it easier to understand at a glance who made the commit
  • Feature has been requested by folks in the community

Possible Drawbacks

  • If folks need the email address of a committer for some reason, now it would take more effort to find it.

Applicable Issues

#1875

Metrics

N/A

Tests

  • Manually tested that committer name shows up as expected for single commits
  • Commiter name shows up as expected for co authored commits
  • name falls back to something reasonable (such as email address or GitHub username) if committer name is not set in git config
  • all new lines covered by unit tests

Documentation

N/A

Release Notes

TBD

User Experience Research (Optional)

N/A

@codecov
Copy link

codecov bot commented Jan 12, 2019

Codecov Report

Merging #1906 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1906      +/-   ##
==========================================
+ Coverage    92.1%   92.11%   +0.01%     
==========================================
  Files         189      189              
  Lines       10779    10783       +4     
  Branches     1580     1580              
==========================================
+ Hits         9928     9933       +5     
+ Misses        851      850       -1
Impacted Files Coverage Δ
lib/models/commit.js 92.23% <100%> (+0.31%) ⬆️
lib/views/commit-detail-view.js 100% <100%> (ø) ⬆️
lib/git-shell-out-strategy.js 100% <100%> (ø) ⬆️
lib/atom/gutter.js 92.3% <0%> (+2.56%) ⬆️

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 d8f360e...825e36e. Read the comment docs.

@annthurium
Copy link
Author

hey @simurai -- this isn't urgent, but if you feel like making this look better, that'd be cool.

Right now it's like:
screen shot 2019-01-11 at 4 46 20 pm

@annthurium
Copy link
Author

Note for future Tilde or whomever ends up picking this up...

I tried testing the case where the commiter name does not exist by unsetting my email, username, and github handle in my git config. Git won't let you commit without one of those things set.

If I unset my github user alias, somehow name is falling back to annthurium. So weird! I don't think it's being pulled from my email address either, because I tried modifying that. Needs further investigation.

@smashwilson
Copy link
Contributor

From now on, all of my commit messages will be "hunks hunks hunks"

@simurai
Copy link
Contributor

simurai commented Jan 15, 2019

How about replacing the current email with the author name and not have both?

image

And only use the email as a fallback if name or username doesn't exist?

Then the case of multiple authors should also be covered. E.g. [name] and 3 others committed....

@annthurium
Copy link
Author

@simurai I like that! I'll move this in that direction.

@annthurium annthurium changed the title [wip] add committer name to commit detail pane add committer name to commit detail pane Jan 15, 2019
@annthurium annthurium requested a review from a team January 15, 2019 23:22
@annthurium annthurium merged commit b509125 into master Jan 16, 2019
@annthurium annthurium deleted the tt-19-jan-commit-detail-polish branch January 16, 2019 19:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants