Add setting to show commit SHAs in tree - #8840
Add setting to show commit SHAs in tree#8840James Miller (jameswilmiller) wants to merge 6 commits into
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/view/treeNodes/commitNode.ts:51
- The description is computed once in the constructor, so toggling
showCommitShaInTreemay not update existingCommitNodeinstances even if the parent refreshes. To ensure the UI reflects the current setting, compute/updatedescriptioninsidegetTreeItem()(or recompute on refresh) rather than only in the constructor.
this.description = this._getDescription();
}
private _getDescription(): string | undefined {
const date = this.commit.commit.author?.date ? dateFromNow(this.commit.commit.author.date) : undefined;
if (!vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get<boolean>(SHOW_COMMIT_SHA_IN_TREE, false)) {
return date;
}
const shortSha = this.commit.sha.substring(0, 7);
return date ? `${shortSha} · ${date}` : shortSha;
}
src/view/treeNodes/commitsCategoryNode.ts:48
- Use the standard capitalization 'SHA' instead of 'Sha' in the log message.
Logger.appendLine(`Commit Sha display setting has changed, refreshing Commits node`, PR_TREE);
package.nls.json:193
- This description string is missing a trailing period, which is inconsistent with nearby setting descriptions.
"githubPullRequests.showCommitShaInTree.description": "Shows the abbreviated commit SHA in the tree view",
src/view/treeNodes/commitNode.ts:41
- The leading underscore is typically redundant on
privatemethods in TypeScript and can be inconsistent with common conventions. Consider renaming togetDescription()(orcomputeDescription()) for clarity and consistency.
private _getDescription(): string | undefined {
…itilsation + trailing period
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/view/treeNodes/commitNode.ts:46
- The new setting-driven formatting logic in
_getDescriptionis not covered by automated tests. Since the repo already has unit tests for tree nodes undersrc/test/view/treeNodes, it would be good to add a focused test verifying the description output for (1) setting disabled, (2) enabled + date, and (3) enabled + no date.
private _getDescription(): string | undefined {
const date = this.commit.commit.author?.date ? dateFromNow(this.commit.commit.author.date) : undefined;
if (!vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get<boolean>(SHOW_COMMIT_SHA_IN_TREE, false)) {
return date;
}
const shortSha = this.commit.sha.substring(0, 7);
return date ? `${shortSha} · ${date}` : shortSha;
src/view/treeNodes/commitNode.ts:48
CommitNodealready storesshaon the instance (this.sha = commit.sha), but_getDescriptionre-reads it fromthis.commit.sha. Usingthis.shaavoids duplication and keeps the source of truth consistent.
const shortSha = this.commit.sha.substring(0, 7);
return date ? `${shortSha} · ${date}` : shortSha;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/view/treeNodes/commitNode.ts:46
- New config-driven description formatting is introduced here but isn’t covered by unit tests. The repo already has Mocha tests for tree nodes under
src/test/view/treeNodes/and patterns for stubbingvscode.workspace.getConfiguration(e.g.src/test/issues/stateManager.test.ts). Consider adding aCommitNodetest that verifies: (1) setting disabled => description is just the relative date/undefined, (2) setting enabled + date =><shortSha> · <relativeDate>, (3) setting enabled + no date =><shortSha>(use fake timers to makedateFromNowdeterministic).
if (!vscode.workspace.getConfiguration(PR_SETTINGS_NAMESPACE).get<boolean>(SHOW_COMMIT_SHA_IN_TREE, false)) {
return date;
}
const shortSha = this.sha.substring(0, 7);
return date ? `${shortSha} · ${date}` : shortSha;
Alex Ross (alexr00)
left a comment
There was a problem hiding this comment.
Thanks for the PR! This looks good, but I'm not sure we actually need a setting for this: let's just always show the commit sha exactly as you have it. If enough folks don't like it, I'll bring back your setting.
| private _getDescription(): string | undefined { | ||
| const date = this.commit.commit.author?.date ? dateFromNow(this.commit.commit.author.date) : undefined; | ||
| const shortSha = this.sha.substring(0, 7); | ||
| return date ? `${shortSha} · ${date}` : shortSha; | ||
|
|
||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/view/treeNodes/commitNode.ts:44
- The new description formatting is applied unconditionally, so commit SHAs will show even when the proposed setting is disabled (and commits without a date will now always show a SHA). This contradicts the PR description/default behavior and changes the existing UI when the setting is off.
Consider gating the SHA prefix behind a configuration check (and returning the original date-only/undefined description when disabled). Also, the setting key doesn’t appear to exist elsewhere in the repo yet (no matches for showCommitShaInTree), so this code should be resilient to the setting being absent (treat as disabled).
private _getDescription(): string | undefined {
const date = this.commit.commit.author?.date ? dateFromNow(this.commit.commit.author.date) : undefined;
const shortSha = this.sha.substring(0, 7);
return date ? `${shortSha} · ${date}` : shortSha;
| private _getDescription(): string | undefined { | ||
| const date = this.commit.commit.author?.date ? dateFromNow(this.commit.commit.author.date) : undefined; | ||
| const shortSha = this.sha.substring(0, 7); | ||
| return date ? `${shortSha} · ${date}` : shortSha; | ||
|
|
Fixes #8701
I wasn't sure whether community contributions were being accepted but I took the liberty of making the PR.
Summary:
Adds a new setting that displays abbreviated commit SHAs in the pull request commits tree.
When enabled, commit descriptions are shown in the following format:
49334c4 · 2 days ago
I went with this design decision because date will truncate before SHA which I believe is the more important
piece of information when reviewing the commits as a list. If you believe a different direction would be better feel free to comment.
The setting defaults to disabled, so the existing behaviour remains unchanged.
Changes:
Testing:
manually verified that:
No automated tests were added ( I wasn't sure whether this was required but I am happy to write some if needed ).
I have attached screenshots below,
Screenshots
Setting:

Disabled Setting:

Disabled Setting with no date:

Enabled Setting:

Enabled Setting with no date:
