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

Removed the default registration of the IHelpProvider #1362

Merged
merged 6 commits into from
Nov 10, 2023

Conversation

nils-a
Copy link
Contributor

@nils-a nils-a commented Nov 10, 2023

fixes #1313

  • I have read the Contribution Guidelines
  • I have commented on the issue above and discussed the intended changes
  • A maintainer has signed off on the changes and the issue was assigned to me
  • All newly added code is adequately covered by tests
  • All existing tests are still running without errors
  • The documentation was modified to reflect the changes OR no documentation changes are required.

Changes

This was a bit more complicated than I expected:
After implementing the changes discussed in #1313 (comment) some of our own tests failed because the FakeTypeRegistrar/FakeTypeResolver did not adhere to our assumptions.

Thus, I created a unit test that covers the FakeTypeRegistrar using the TypeRegistrarBaseTests and resolved all issues accordingly.
Also, there was a minor issue in the ComponentRegistry that arose when mixing Register, RegisterInstance and RegisterLazy - not sure if mixing Register with RegisterInstance while using the same interface is an issue that will occur in the "real world", but it's fixed now 😄
Also there was something going on with line endings in CommandExecutor, not sure why that was so.

@nils-a nils-a requested a review from a team November 10, 2023 22:23
All of which I introduced earlier.
@nils-a nils-a changed the title Feature/gh 1313 Removed the default registration of the IHelpProvider Nov 10, 2023
Copy link
Contributor

@patriksvensson patriksvensson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@patriksvensson patriksvensson merged commit 023c77f into spectreconsole:main Nov 10, 2023
5 checks passed
@FrankRay78
Copy link
Contributor

Nice PR @nils-a

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.

TypeRegistrar and custom HelpProvider cannot be used together
3 participants