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

Add protected BaseResult() method to CallInfo. #641

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dtchepak
Copy link
Member

Create CallInfo to calls that return results and expose GetBaseResult.
This gets messy to push the generic all the way through the code, so am
just using a cast in Returns extensions to handle this. This should be
safe as if we are in Returns<T> then the return value should be safe to
cast to a T.

Based off discussion here:
#622 (comment)

@dtchepak dtchepak requested a review from zvirja January 11, 2021 04:24
@dtchepak
Copy link
Member Author

@zvirja This is my attempt at implementing your suggestions from #622. If this seems ok I can try to do something similar for When..Do if you like, but that will be trickier.

@alexandrnikitin
Copy link
Member

@dtchepak I'm a bit out of context after absence and need more time to ramp up 😊 A couple of questions.
Can we limit it to Partial mocks only. So that in regular substitutes it's not possible to call base. At first look it makes sense not to mix them.
Would it be better if we make a breaking decision to not-call-base by default and expose that via API?

@dtchepak
Copy link
Member Author

Can we limit it to Partial mocks only.

I don't think it is possible to distinguish partial mocks statically using our API. 😞

Would it be better if we make a breaking decision to not-call-base by default and expose that via API?

I'm still not sure on this all these years later! 😂 Leaving partial mocks out of this for the moment, I think it makes sense to be able to access the base value from within the Returns callback. At present the code (should) throw if there is no base implementation, so in cases where it does not make sense to call base people will immediately be informed via their test.

@dtchepak
Copy link
Member Author

@tpodolak Will this change to use CallInfo<T> cause any problems for Analyzers?

@macsux
Copy link

macsux commented Jan 24, 2021

Would like to see this merged in as I need this functionality too

@tpodolak
Copy link
Member

tpodolak commented Jan 24, 2021

@tpodolak Will this change to use CallInfo<T> cause any problems for Analyzers?

I think it might. Analyzers do some check against CallInfo type. For instance, they analyze ArgAt method usage only if this method is declared in CallInfo type. CallInfo will not fall into that category (at least from perspective of current analyzers codebase). I will try to double check this week, but I am quite sure that some diagnostics might not be reported

@dtchepak
Copy link
Member Author

@tpodolak CallInfo<T> inherits from CallInfo; not sure if that will help?

@tpodolak
Copy link
Member

tpodolak commented Jan 27, 2021

@tpodolak Will this change to use CallInfo<T> cause any problems for Analyzers?

I think it might. Analyzers do some check against CallInfo type. For instance, they analyze ArgAt method usage only if this method is declared in CallInfo type. CallInfo will not fall into that category (at least from perspective of current analyzers codebase). I will try to double check this week, but I am quite sure that some diagnostics might not be reported

@dtchepak I've built your branch and run analyzers against that and as expected, callInfo warnings are gone

  • NSubstitute 4.2.2
    before
  • CallInfo version
    after
    image

@tpodolak CallInfo inherits from CallInfo; not sure if that will help?

Thanks, I've seen that, however this still breaks current CallInfo alayzers. I will fix analyzers part once this is merged and published to NuGet or some private feed

@dtchepak
Copy link
Member Author

Thanks @tpodolak . Is it possible to support both? Or will different NSub versions require different Analyzer versions?

@dtchepak
Copy link
Member Author

@zvirja Are you OK with this change? If you don't have time to look at it please let me know. 😄

@tpodolak
Copy link
Member

tpodolak commented Jan 30, 2021

Thanks @tpodolak . Is it possible to support both? Or will different NSub versions require different Analyzer versions?

It is possible to support both versions with just one version of analyzers. Here are necessary chagnes nsubstitute/NSubstitute.Analyzers#158

@zvirja
Copy link
Contributor

zvirja commented Feb 3, 2021

Hi @dtchepak! Hope you are doing fine and are not annoyed too much that I'm not replying timely 😖

I actually have a bit of concerns regarding the newly introduced CallInfo<T>. When working on nullability annotations I found that the separation between internal (core) and external parts of NSubstitute is not clear. Sometimes we have public-facing API which are returning low-level Core entities.

In order to enhance that I actually was planning to revisit it the next major version. The idea was that we e.g. have public interfaces for things like CallInfo, Call and never return exact implementations.

The motivation is that we could modify Core more freely without worrying to affect public surface. Also we could more clearly annotate code with nullability attributes - public surface not annotated, private - annotated entirely.

Same for consumption - if you consume anything from Core namespace - expect things to be more fragile and volatile.

Not sure whether you share my vision on this. Library is really small, so such kind of things might be an overkill. But somehow I see a beauty and usefulness in that, as we are just a few tiny steps from there.

@dtchepak
Copy link
Member Author

dtchepak commented Feb 3, 2021

@zvirja

Hope you are doing fine and are not annoyed too much that I'm not replying timely

Of course not annoyed! We're all volunteers here, I am grateful for any reply!

In order to enhance that I actually was planning to revisit it the next major version. The idea was that we e.g. have public interfaces for things like CallInfo, Call and never return exact implementations.

Would you be happier if I change this PR to expose interfaces for these? Alternatively we could take this as-is (if you are happy enough with the implementation) and do the larger change to interfaces in a separate issue?

We should also be mindful that any change along these lines will probably make a lot of work for the @tpodolak and the Analyzers project.

@zvirja
Copy link
Contributor

zvirja commented Feb 15, 2021

Hi @dtchepak,

Would you be happier if I change this PR to expose interfaces for these?

Yes, I think it would be nice to start with interfaces approach for new features, so later we could rework other features to catch-up. This way we can also see how it looks like.
I would put interface somewhere outside of Core, but the implementation could be in Core somewhere.

dtchepak added a commit to dtchepak/NSubstitute that referenced this pull request Mar 28, 2021
Create CallInfo<T> to calls that return results and expose `BaseResult`.
This gets messy to push the generic all the way through the code, so am
just using a cast in `Returns` extensions to handle this. This should be
safe as if we are in `Returns<T>` then the return value should be safe to
cast to a `T`.

Based off discussion here:
nsubstitute#622 (comment)
@dtchepak
Copy link
Member Author

@zvirja I've attempted to extract ICallInfo into root namespace. Can you please re-review?

I'm a bit concerned about the ICallInfo.ForCallReturning<T> but wasn't sure how best to do this. (I think this is better than casting to the internal CallInfo type though.)

@zvirja
Copy link
Contributor

zvirja commented Apr 25, 2021

@dtchepak I've gave it a brief look and before we proceed further, I can see that we introduce a lot of breaking changes to API (as use now use interface instead of implementation). I'm afraid that it could break other libraries (like AutoFixture) which might depend on these APIs. Should we then consider branching out v5 and releasing a new major version with this?

Then we could add a couple of other improvements we planned to add.

@dtchepak
Copy link
Member Author

Thanks @zvirja. We are fine for next release to be 5.0.0 I think. We tend not to branch for that; develop will just be tracking 5.x now. I'm happy to put more process around this if you prefer?

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.

5 participants