Read on Twitter
after 2 days, finally got to the bottom of a very weird bug with a rails library we& #39;re using that boils down to whether a method is intended to
A) cause a certain change, or
B) bring about a certain state
incredibly mad about this
A) cause a certain change, or
B) bring about a certain state
incredibly mad about this
basically the library supports & #39;add& #39; and & #39;delete& #39; operations on its config, except & #39;delete& #39; is not the inverse of & #39;add& #39;; whereas & #39;add& #39; adds one thing to a list, & #39;delete& #39; removes _all_ occurrences of it
which means usage like this doesn& #39;t work
config.add(:thing)
# do some work under the modified config
config.delete(:thing)
config.add(:thing)
# do some work under the modified config
config.delete(:thing)
your intent here is *probably* to temporarily ensure :thing is in the config, removing it afterward _if it wasn& #39;t there to begin with_
what you actually get is that :thing is unconditionally purged at the end, even if it was present before this code ran
what you actually get is that :thing is unconditionally purged at the end, even if it was present before this code ran
what you actually want here is an API like
config.with(:thing) do
# do your work
end
or,
effect = config.add(:thing)
# do the work
effect.undo
i.e. the add->work->delete workflow makes the assumption that & #39;add& #39; actually changes something when it doesn& #39;t
config.with(:thing) do
# do your work
end
or,
effect = config.add(:thing)
# do the work
effect.undo
i.e. the add->work->delete workflow makes the assumption that & #39;add& #39; actually changes something when it doesn& #39;t
plus, you& #39;re mutating something and yielding to arbitrary code in the middle of it, which is always bound to go wrong
this could be mitigated by `config` internally using a Set instead of an Array internally, and returning a boolean from `add`, but ergonomically it would be hard to force the caller to use the result
better to have an API that actually enforces the semantics
better to have an API that actually enforces the semantics
what you have instead is that if config contains [:apples, :oranges], then:
config.add(:apples)
# => config == [:apples, :oranges, :apples]
config.delete(:apples)
# => config == [:oranges]
so I& #39;ve not returned the object to its original state
config.add(:apples)
# => config == [:apples, :oranges, :apples]
config.delete(:apples)
# => config == [:oranges]
so I& #39;ve not returned the object to its original state
the config is used by checking whether it contains certain symbols, so this could be fixed either by only removing the last occurrence of a symbol, or using a set, or changing the API
but then what if I meant "definitely turn this feature off" when I call `delete`?
but then what if I meant "definitely turn this feature off" when I call `delete`?
this is why you can& #39;t just expose a bunch of arbitrary mutation operations on structures -- you have to encode use cases and workflows
is & #39;delete& #39; intended to let you temporarily ensure a feature is active, letting you turn it off afterward if it was not already active, or is it intended for turning the feature off? it& #39;s not documented so who knows
there is an API for getting the current state of the config, but it& #39;s not documented either and very much appears to be intended for internal use, so I wouldn& #39;t use that to check if the feature is active first either
when you do API design, come back to the first question I had in this thread: should this method cause a change to happen, or should it bring about a certain state? how are you communicating that?
