Refactor Internal module

[vburzynski]

I experimented with refactoring the FactoryBot::Internal and FactoryBot::Configuration code:

• Previously the query methods in Internal had to reach into Configuration to access the registry instances and other config state. These methods also had low cohesion as they all used distinct subsets of the configuration state.
• This refactor encapsulates the query methods with the state/registries that they are dependent upon and places them in independent modules.
• The Internal module remains as a proxy/facade/aggregated type interface to minimize the impact on other modules throughout FactoryBot
• the refactoring obviates the need to have the Configuration class and create an instance of it.
• Constructor#initialize_with was a alias/delegation of Definition#define_constructor so initialize_with is now an alias inside the Definition

[neilvcarvalho]

Now this class has both clear and reset, which redefines the ivar or calls clear on it. Should we call clear instead of reset? I don't have a strong opinion on either.

[vburzynski]

oh good question 🤔 I implemented the #reset method to break up behavior previously found in Internal#reset_configuration; in doing so, I had overlooked that there was a #clear here.

At the outset of considering the two methods, I don't have a strong opinion for using one over the other. I do believe we should normalize and standardize things for clarity.

• Ruby core classes like Hash and Array have a #clear method.
  • So a #clear method would seem to be a more idiomatic name to use.
• the performance difference between Hash.new and Hash#clear is likely negligible for most use cases. I'm not quite clear on what the impact on garbage collection would be. I'll assume any impact there is also negligible.
• Considering the behavior nuances between the two methods: nothing outside of the Registry should be keeping a reference to the @items ivar.
  • So in this regard, I think either #reset or #clear is fine to use.
  • When Hash#clear is invoked, the hash itself is returned. Yet Registry#clear doesn't return self, but the result of @items.clear (which is @items). It is theoretically possible for something to call Registry.clear and then store a reference to the hash that's referenced by @items.
  • This is relevant because when usingRegistry#clear, all references to the hash value will be cleared.
  • Registry#reset, on the other hand, sets @items to be a new Hash, and all references to the previous hash will remain unaffected.
  • once again, however, nothing should be keeping a reference to @items and we don't do this inside FactoryBot.

∴ I'd go with #clear over #reset

# create a new hash
irb(main):001> a = { a: 1 }
=> {a: 1}

# set b as a reference to the hash after it is cleared
irb(main):002> b = a.clear
=> {}

# a and b now reference the same hash value
irb(main):003> a[:c] = 3
=> 3
irb(main):004> a
=> {c: 3}
irb(main):005> b
=> {c: 3}

# setting a to be a new Hash leaves be unaffected
irb(main):006> a = { b: 2 }
=> {b: 2}
irb(main):007> a
=> {b: 2}
irb(main):008> a.clear
=> {}
irb(main):009> a
=> {}
irb(main):010> b
=> {c: 3}

[neilvcarvalho]

I like it overall. There are more module variables to think about now, but I think it's worth it.