Skip to content

Move some core type into internal & migrate to slnx #880

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

Closed
wants to merge 5 commits into from

Conversation

Romfos
Copy link
Contributor

@Romfos Romfos commented May 15, 2025

Changes:

This PR is a follow up for discussion #869 and alternaltive solution when we don't what make these types "true" internal\sealed but still want to logically separate these types

Core types that are still "public" core namespace:

  1. Interfaces
  2. Container, Context and di stuff
  3. Some core types that were referenced in tests (goal was to make NSubstitute.Acceptance.Specs to have 0 references to internal namespace)

Related #830

@Romfos Romfos mentioned this pull request May 15, 2025
15 tasks
Copy link
Contributor

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

There are multiple internal types leaking into the public API,
See #881

I do think we should keep internal types, internal. Do you agree @Romfos ?

Update: I think most of the "violations" could be easily fixed with an public interface

@@ -10,7 +10,7 @@ ICalculator calculator;
```
-->

The `Throws` and `ThrowsAsync` helpers in the `NSubstitute.ExceptionExtensions` namespace can be used to throw exceptions when a member is called.
The `Throws` and `ThrowsAsync` helpers in the `NSubstitute.Extensions` namespace can be used to throw exceptions when a member is called.
Copy link
Contributor

@304NotModified 304NotModified May 15, 2025

Choose a reason for hiding this comment

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

Is this some other change, as it isn't the internal namespace?
Are more namespaces changed in this PR?

@304NotModified 304NotModified changed the title Move some core type into internal Move some core type into internal & migrate to slnx May 17, 2025
@dtchepak
Copy link
Member

Initially the NSubstitute.Core namespace was meant to be internal/use-at-your-own-risk. NSubstitute was for the public API. Is the NSubstitute.Internal change just to make this more specific?

The MR looks good, will approve pending feedback on @304NotModified's comments. 👍

@304NotModified
Copy link
Contributor

@dtchepak
What's a MR? Do you mean PR?

@dtchepak
Copy link
Member

@dtchepak What's a MR? Do you mean PR?

Haha, yes, sorry. I do a lot with gitlab where they're known as Merge Requests.

@Romfos
Copy link
Contributor Author

Romfos commented May 20, 2025

@304NotModified thank you for your feedback.
the more I go deep to unit test that @304NotModified mention then more I understand how crazy this feature will be

I'm afraid that this approach will make more problems then solve
Original idea was to simplify future changes and make support easier

Lets save status quo for now and don't do anything with it

@Romfos Romfos closed this May 20, 2025
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