Update behavior of ObjectContainer.IsRegistered() to check base container for registrations

[DrEsteban]

🤔 What's changed?

Added an IsRegisteredAtAnyLevel() method to ObjectContainer, so users can check if a type is registered at any level of the container hierarchy.

⚡️ What's your motivation?

#366

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

Nothing in particular!

📋 Checklist:

• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

[DrEsteban]

@obligaron @gasparnagy Any feedback on this idea or implementation?

[obligaron]

lgtm

I'm just not sure if "current level of container hierarchy" or "any level of container hierarchy" is the best naming convention. Maybe "current container" or "current and base container" would be better. But I'm not sure, naming is hard. 😅

[DrEsteban]

Thanks @obligaron! That's totally fair, let me make that update.

Just curious, how do PRs normally work on this project? Who/what merges them? Also, regarding the warning about "This branch must not contain merge commits", I don't believe my branch has any such commits... Is that something I need to resolve? Or is that something maintainers will resolve by selecting Squash Merge for example?

[gasparnagy]

@DrEsteban This section should answer the contribution related questions. (Please review and suggest updates if not clear.)

Merge commits are fine, we always squash the PRs when merging.

[gasparnagy]

Thank you for your the contribution. Good stuff.

We had some discussion with @Code-Grump about the namings and the interface structure.

Our suggestion is the following:

1. Instead of adding an additional method to the interface, we would like to change the existing behavior of the IsRegistered methods. (Based on the behavior of the other methods, like Resolve, the behavior that checks the parent as well makes more (only?) sense.)
2. In the implementation, if you need to create a private helper method for checking it only in the current container, you can use the name 'IsDirectlyRegistered'.
3. We have reviewed the existing usages within the project and they should be fine with the new behavior too.
4. If anyone needs the existing behavior, they can apply c. IsRegistered<t>() && !((ObjectContainer)c).Parent.IsRegistered<t>() if needed and of course later we can consider extending the interface with it if needed.

Please let me know if something is unclear.

[DrEsteban]

Just wanted to drop a note that I definitely plan on addressing this! I'm just coming off a particularly demanding on-call rotation at my day job 😄🥲

[DrEsteban]

Okay! Updated.

As discussed, a behavior change was made to IsRegistered() to check base containers for registrations instead of introducing new methods to the interface. I also updated the unit tests to better represent how IsRegistered() inheritance is supposed to work after this change - validating structures that (I believe) represent the 90%+ case of usage patterns. CHANGELOG was also updated accordingly.

Method recursion was used for the implementation - where ObjectContainer simply invokes the IsRegistered() method of its base. This way, if users choose to override the implementation of IObjectContainer at any layer, they'd have the ability to intercept the call and handle the responsibility of recursion however they wish. (Previously, the IsRegisteredAtAnyLevel() implementation didn't leave as much flexibility to interrupt/customize the flow of control since it explicitly tried to cast to ObjectContainer.) Given that normal usage of Reqnroll involves only 3 (4?) "layers" of containers, I didn't consider stack overflow to be a concern here.

Let me know if you have any additional questions or concerns!

[DrEsteban]

Given this is a "breaking"/behavior change, is there any major/minor version specification I should be revving as part of this PR? Or will maintainers handle that as part of the next release?

Is this message clear enough to indicate it's a behavior change? I see historical change entries for bug fixes have a Fix: prefix, and new features/support seem to not have anything. Should there be a Breaking: (or similar) prefix to this one? (It could be argued it's a Fix for behavior it should have always had...or a "design bug fix"...but it's not really a traditional code bug.)

If you'd like any of the above☝️ concepts solidified, let me know and I can create another PR to update the CONTRIBUTING.md.