Skip to content
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

[WIP] Fix property assignment in primitives example #6355

Closed
wants to merge 2 commits into from
Closed

[WIP] Fix property assignment in primitives example #6355

wants to merge 2 commits into from

Conversation

user135711
Copy link
Contributor

Fixed a bug where CurrentState is assigned the wrong value on each invocation followed by being assigned the correct value in index. Also fixed an np completeness inefficiency by passing a lamda instead of a method group since I was editing the line, and used a non primitive state object so that the value is passed by reference to reflect the current state. For np explanation see
hereand
here

@user135711 user135711 changed the title Master Fix property assignment in primitives example May 11, 2018
@user135711
Copy link
Contributor Author

user135711 commented May 11, 2018

I didn't realize that github would squash any commit into my personal repository into any pull requests so this commit now has an unrelated commit included which makes webcache compilable. The second commit fixes #6299 comilation.

@guardrex
Copy link
Collaborator

Hello @user135711! Thanks for submitting this.

Would you mind removing the changes for the WebCache sample from this? We wouldn't want both samples updated on the same PR, and we can't take a PR for the WebCache sample at this time. For one thing, we can't upgrade it to 2.1 until 2.1 RTM. Also, it's due for a significant overhaul when it is upgraded that will go beyond just making it run under 2.1. We're tracking the work for it on #5495.

@guardrex
Copy link
Collaborator

RE: The ChangeTokens updates

The changes proposed require matching changes to the topic's text for the ConfigurationMonitor ctor and the InvokedChange callback on the same PR so that the live sample doesn't become out-of-sync with the topic when the PR is merged.

https://docs.microsoft.com/aspnet/core/fundamentals/primitives/change-tokens#monitoring-configuration-changes-as-a-service

@guardrex guardrex changed the title Fix property assignment in primitives example [WIP] Fix property assignment in primitives example May 11, 2018
@guardrex guardrex added the WIP label May 11, 2018
@user135711
Copy link
Contributor Author

@guardrex That's a good point, thanks...

@user135711 user135711 closed this May 11, 2018
@guardrex
Copy link
Collaborator

If you prefer that this just be taken into account for the Change Tokens 2.1 update, I recommend just opening an issue referencing this, then I'll add a note to the tracking issue for it.

@guardrex
Copy link
Collaborator

guardrex commented May 11, 2018

Alternatively, feel free to go ahead with a PR on the Change Tokens topic+sample with these changes now, and I'll ping engineering to take a look.

@user135711
Copy link
Contributor Author

user135711 commented May 11, 2018

@guardrex Shrug, if you're doing it anyway, I'll just let you update them at once. I do have some suggestions for the web cache update that I'll post directly to the issue so you'll consider including it and save me some time if you do it.

@guardrex
Copy link
Collaborator

ok ... sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants