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

Provide a non-threadsafe mechanism to override registered values #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

billdueber
Copy link

Uses a simple push/pop with the current context (registry, cache, etc.) to override withing the push/pop cycle, or use container.override { ... } to automatically push/pop at the beginning and end of the block.

Did I say "non-threadsafe?" OVERRIDES ARE NOT THREADSAFE!

Uses a simple push/pop with the current context (registry,
cache, etc.) to override withing the push/pop cycle, or use
`container.override { ...  } ` to automatically push/pop at the
beginning and end of the block.

Did I say "non-threadsafe?" OVERRIDES ARE NOT THREADSAFE!
@billdueber
Copy link
Author

Geez. Realized I didn't commit a couple files. Fixed.

Copy link
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

This looks pretty nice; I want to give it a try with some of my projects where I override things in tests to make sure that it works as expected.

@moseshll
Copy link
Contributor

moseshll commented Nov 9, 2023

Spent a bit of time with this branch and hathitrust_catalog_indexer and overall I'm liking it. There is some behavior in this branch that I believe violates the principle of least astonishment, however. To wit:

irb(main):002:0> c = Canister.new                                                    
irb(main):003:0> c.push_context!                                               
irb(main):004:0> c.register(:a) { "b" }                                                                         
irb(main):005:0> c.pop_context!                                                                          
irb(main):006:0> c[:a]
=> "b"

Expected behavior would, for me, be a raised exception since the registration was made in a pushed context.
@context.dup defaults to a shallow copy of the existing context, so the @registry hash is shared between all contexts. I tried this and was satisfied with the behavior:

def dup
  self.class.new(registry: @registry.dup)
end

(I should also note that doing c[:some_key_i_havent_registered_yet] leaves junk on the @stack but that's an issue for another day.)

Copy link
Contributor

@moseshll moseshll left a comment

Choose a reason for hiding this comment

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

See main comment. (Figured I might as well turn this into a real review.)

Probable Scope Creep Initiate!
I also find the cited behavior of e.g., c[:some_key_i_havent_registered_yet] (raising NoMethodError and leaving junk on the stack) somewhat jarring. Surely we can act more like Hash (which is ultimately how I conceptualize this class) or at least return a more appropriate exception class.

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.

3 participants