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

Cache release notes. Fixes #13257 #15050

Merged
merged 1 commit into from Dec 13, 2016

Conversation

Projects
None yet
3 participants
@olingern
Contributor

olingern commented Nov 5, 2016

No description provided.

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Nov 5, 2016

Hi @olingern, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas commented Nov 5, 2016

Hi @olingern, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@joaomoreno

Sorry about the lack of guidance so far.

When I said keep them in memory I literally meant it as a variable in some scope. Don't use the storage service, since it will polute the storage and we don't care about having this information surviving between executions.

Create an object, next to loadReleaseNotes that will serve as a cache and use version as a key for that cache.

@olingern

This comment has been minimized.

Show comment
Hide comment
@olingern

olingern Nov 8, 2016

Contributor

No problem!

I refactored to use a string instead of the storage service.

Is there a reason you'd prefer the cache be an object with a key over a regular string? Is there a use case where multiple versions would be cached and need to be accessed directly on an object?

Contributor

olingern commented Nov 8, 2016

No problem!

I refactored to use a string instead of the storage service.

Is there a reason you'd prefer the cache be an object with a key over a regular string? Is there a use case where multiple versions would be cached and need to be accessed directly on an object?

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Nov 8, 2016

@olingern, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

msftclas commented Nov 8, 2016

@olingern, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@msftclas msftclas added cla-signed and removed cla-required labels Nov 8, 2016

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Nov 8, 2016

Member

Yeah, using the version as a key lets you read the current version's release notes as well as the next one, once you get an update notification.

Member

joaomoreno commented Nov 8, 2016

Yeah, using the version as a key lets you read the current version's release notes as well as the next one, once you get an update notification.

@olingern olingern changed the title from Cache release notes using workspaceStorage. Fixes #13257 to Cache release notes. Fixes #13257 Nov 8, 2016

@olingern

This comment has been minimized.

Show comment
Hide comment
@olingern

olingern Nov 8, 2016

Contributor

@joaomoreno Got it. I updated the PR and rebased commit history to be a bit more clean. Let me know if this all goods good and if there's anything I can do to help get the tests passing.

Contributor

olingern commented Nov 8, 2016

@joaomoreno Got it. I updated the PR and rebased commit history to be a bit more clean. Let me know if this all goods good and if there's anything I can do to help get the tests passing.

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Nov 8, 2016

Member

Looks better! 👍

The problem is you need to merge the latest master onto your branch, do you mind doing this? That way our continuous integration tests will run on the PR.

Member

joaomoreno commented Nov 8, 2016

Looks better! 👍

The problem is you need to merge the latest master onto your branch, do you mind doing this? That way our continuous integration tests will run on the PR.

@olingern

This comment has been minimized.

Show comment
Hide comment
@olingern

olingern Nov 10, 2016

Contributor

@joaomoreno I pulled the latest master branch, but opening the release notes opens in the browser each time now.

Contributor

olingern commented Nov 10, 2016

@joaomoreno I pulled the latest master branch, but opening the release notes opens in the browser each time now.

@joaomoreno joaomoreno added this to the November 2016 milestone Nov 10, 2016

@olingern

This comment has been minimized.

Show comment
Hide comment
@olingern

olingern Nov 18, 2016

Contributor

@joaomoreno opting to close this as I see the latest version of Insider's release opens in the browser.

Contributor

olingern commented Nov 18, 2016

@joaomoreno opting to close this as I see the latest version of Insider's release opens in the browser.

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Nov 21, 2016

Member

@olingern The insider release will always open in the browser. The stable one opens in the product.

Member

joaomoreno commented Nov 21, 2016

@olingern The insider release will always open in the browser. The stable one opens in the product.

@joaomoreno joaomoreno modified the milestones: January 2017, November 2016 Dec 6, 2016

@joaomoreno joaomoreno merged commit 5c5a630 into Microsoft:master Dec 13, 2016

1 check failed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Dec 13, 2016

Member

Thanks for the PR!

Member

joaomoreno commented Dec 13, 2016

Thanks for the PR!

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