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

Support indexer with multiple arguments #694

Merged
merged 2 commits into from
Oct 8, 2018

Conversation

idigra
Copy link
Contributor

@idigra idigra commented Sep 18, 2018

This PR contains support in multiple arguments.

  1. Fix null reference exception in case of multiple arguments indexer.
  2. Proposal for new API method SetupSetMethod which allows using It.IsAny for the indexer arguments (can be used also for properties, although existing API already covers properties). This is only in proposal level, should be enhanced (documentation, overloading for any count of indexer arguments, exceptions on illegal input).

@stakx
Copy link
Contributor

stakx commented Sep 18, 2018

Hi @idigra, and thanks for this PR, too. Same as with the earlier one from today, I'll try to find some time for reviewing this soon.

Fix null reference exception in case of multiple arguments indexer.

Which null reference exception are we talking about here? Could you post an issue with repro code (or refer me to an existing closed issue)?

Proposal for new API method SetupSetMethod [...]

It shouldn't be necessary to introduce a SetupSetMethod. You'll find that the existing Setup, SetupGet, SetupSet etc. methods already try to recognize when they are called on indexers—indexers shouldn't have their own dedicated methods, from a user's perspective that doesn't make much sense. (SetupGet and SetupSet are probably already confusing enough.)

The problem with It.IsAny and SetupSet would be easily solvable by bringing back the old, deprecated SetupSet overload that takes two expressions: one for the property/indexer to be set up, and one for the value. But unfortunately, that overload has been marked deprecated. I think we should rather de-deprecate the existing method, instead of introducing a new one (if we do something at all).

@idigra
Copy link
Contributor Author

idigra commented Sep 20, 2018

Which null reference exception are we talking about here? Could you post an issue with repro code (or refer me to an existing closed issue)?

I added a test in the PR that reproduces the bug, named CallbackWithMultipleArgumentIndexerSetterWithoutAny. Now there's an issue #695 :)

I think we should rather de-deprecate the existing method, instead of introducing a new one (if we do something at all).

Why not? I think it's a useful scenario.
(BTW, even if you think not to de-deprecate, we should solve following issue: #696).

@idigra
Copy link
Contributor Author

idigra commented Sep 20, 2018

I now see that the second issue I opened (#696, mentioned above) is a duplicate of #218 which is closed in favor of #414. I couldn't find the reason why the SetupSet overload you mentioned was deprecated. Also consider the solution I proposed in my issue #696.

@stakx
Copy link
Contributor

stakx commented Sep 20, 2018

I couldn't find the reason why the SetupSet overload you mentioned was deprecated.

I don't know the exact reason, either. I am guessing that this decision was made simply because being able to write SetupSet(m => m.Something = value) is nicer than SetupSet(m => m.Something, () => value).

Copy link
Contributor

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR! 👍

I'd appreciate if we could focus only on your PR's first stated goal, i.e. fixing the NullReferenceException when setting up an indexer with more than one index parameter. This is the reason why I'm asking you (below) to remove everything related to SetupSetMethod.

I'm happy to discuss SetupSetMethod / the deprecated SetupSet overload / improvements of the current implementation of SetupSetImpl in a separate issue, if you'd like.

Also, please add an entry for your bug fix to CHANGELOG.md:

 # Moq Changelog

 All notable changes to this project will be documented in this file.

 The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).


+## Unreleased
+
+#### Fixed
+
+* Short description of your bugfix (@idigra, #694)
+
+
 ## 4.10.0 (2018-09-08)

src/Moq/Mock.cs Show resolved Hide resolved
tests/Moq.Tests/CallbacksFixture.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/CallbacksFixture.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/CallbacksFixture.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/CallbacksFixture.cs Outdated Show resolved Hide resolved
src/Moq/Mock.Generic.cs Outdated Show resolved Hide resolved
src/Moq/Mock.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Sorry for being quiet so long. All good to go now. I'll update the changelog for you.

src/Moq/Mock.cs Show resolved Hide resolved
@stakx stakx merged commit 065afe7 into devlooped:master Oct 8, 2018
@stakx stakx added this to the 4.10.1 milestone Oct 15, 2018
@stakx
Copy link
Contributor

stakx commented Dec 3, 2018

@idigra, Moq 4.10.1, which includes your bugfix, has just been published on NuGet.

@idigra
Copy link
Contributor Author

idigra commented Dec 28, 2018

Thanks. I finally came to verify it, but then I understand that without extending the API to support Any matcher in indexer argument I can't really reach a scenario in which the bug was reproduced.
I would be happy to check it if you have a real scenario in which the bug occurs.

@stakx
Copy link
Contributor

stakx commented Dec 29, 2018

I can't really reach a scenario in which the bug was reproduced.

Sorry, it's been a while, and there's not much context here. Which bug exactly are you referring to?

@idigra
Copy link
Contributor Author

idigra commented Dec 30, 2018

#696

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.

None yet

2 participants