-
Notifications
You must be signed in to change notification settings - Fork 74
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
chore: show release notes on user demand on remote env #1039
chore: show release notes on user demand on remote env #1039
Conversation
const remote = isRemote(); | ||
if (calledOn === "onExtensionStart" && remote.kind === "left") { | ||
return remote; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes more sense to check version first, only after remote. It yields better message: Release notes was not shown: do not show release notes for an older version/already seen version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minor comments, but otherwise LGTM :)
src/releaseNotesProvider.ts
Outdated
@@ -128,7 +132,9 @@ async function showReleaseNotesImpl( | |||
|
|||
return isNewerVersion | |||
? makeRight(cleanVersion) | |||
: makeLeft("do not show release notes for an older version"); | |||
: makeLeft( | |||
"do not show release notes for an older version/already seen version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"do not show release notes for an older version/already seen version" | |
`do not show release notes for switching to the older or same version, previous version: ${previousVersion}, current version: ${currentVersion}` |
What do you think about this? I personally think "same version" makes more sense than "already seen version" (if I understand correctly). Also, it would be helpful if there's version information in the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, what about do not show release notes for an older or same version, previous version: ${previousVersion}, current version: ${currentVersion}
. switching
seems strange to me 🤔
@ckipp01 can you help us here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm missing some context, so when would a user see this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release notes should be shown only once for particular version (vs code keeps track of the newest one) and not shown for an older version than was already seen.
This message (it's more a debug one) will be logged to the output tab when any of aforementioned conditions are false:
- release notes were showed for current version
- user is using version older than the last one for which notes were showed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, so something like
not showing release notes since they've already been seen for your current version
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to @ckipp01 's comment, my suggestion was too much related to the internal behavior that shouldn't be exposed to end users.
689431f
to
e19c824
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except the log message, LGTM 👍
No description provided.