Skip to content
This repository has been archived by the owner on Nov 7, 2018. It is now read-only.

Options Reloaded #154

Closed
wants to merge 1 commit into from
Closed

Options Reloaded #154

wants to merge 1 commit into from

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Sep 28, 2016

Fixes #145 (comment)

Initial shape with basic tests (want to add a few more)

Few points to consider:

  • The feature is always 'on', any time Configure(IConfiguration) is called, we hook up the notifications (no way to disable)
  • We could have the current OptionsManager implement both IOptions and IOptionsSnapshot (we'd register the class twice, singleton for IOptions, and Scoped for Snapshot, felt too weird so I didn't go that route, and copied the class, since its tiny)
  • Support multiple listeners in OptionsMonitor.OnChange or not? Basically subsequent calls could either add additional callbacks, or override the first callback. I did the more complicated list of callbacks, but I'd be glad to kill this.
  • (Found a nasty bug with CurrentValue in the old implementation where it wouldn't get updated if no one ever called OnChange)

@HaoK
Copy link
Member Author

HaoK commented Sep 28, 2016

@divega
Copy link

divega commented Sep 28, 2016

The feature is always 'on', any time Configure(IConfiguration) is called, we hook up the notifications (no way to disable)

At first glance it looks like the baseline cost (if noone hooks up to listen) is a singleton IOptionsChangeTokenSource. Are there other hidden costs?

Support multiple listeners in OptionsMonitor.OnChange or not? Basically subsequent calls could either add additional callbacks, or override the first callback. I did the more complicated list of callbacks, but I'd be glad to kill this.

I agree the code is a bit more involved this way, but I think it is fine, and I cannot see us getting away without it.

Copy link

@divega divega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but you will probably want to get another round of votes for naming (in case sleeping on it or the 🍺 helped).

@HaoK
Copy link
Member Author

HaoK commented Sep 29, 2016

Yeah I guess the overhead isn't too bad if no one asks for an IOptionsMonitor since the change token hookup in the ctor won't happen

@HaoK HaoK closed this Sep 29, 2016
@HaoK HaoK reopened this Sep 29, 2016
@HaoK
Copy link
Member Author

HaoK commented Sep 30, 2016

@Eilon @DamianEdwards @davidfowl last call for other name suggestions for the initial checkin...?

'

@galich
Copy link

galich commented Sep 30, 2016

Keep IOptionsSnapshot - it conveys the nature of the object ;)

@HaoK
Copy link
Member Author

HaoK commented Oct 12, 2016

acecacf

@RehanSaeed
Copy link

RehanSaeed commented Feb 1, 2017

I fully expected the reason for the existence of IOptions<T> being the reloading ability. Why was IOptionsSnapshot<T> created instead and why not just register the type T directly?

@natemcmaster natemcmaster deleted the haok/9-28reload branch November 16, 2017 05:04
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.

5 participants