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.

Sign up for GitHub

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

Jump to bottom

Improve output for expected argument matchers #806

Merged
merged 2 commits into from
Nov 30, 2024
Merged

Improve output for expected argument matchers #806

merged 2 commits into from
Nov 30, 2024

Conversation

dtchepak
Copy link
Member

@dtchepak dtchepak commented May 5, 2024

  • Add IDescribeSpecification to allow custom arg matchers to provide custom output for "expected to receive" entries.
  • Fallback to ToString when IDescribeSpecification not implemented.
  • Update code comment docs accordingly.

Closes #796.

@dtchepak dtchepak mentioned this pull request May 5, 2024
Closed
@dtchepak dtchepak force-pushed the improve-output-for-expected-arg-match branch 2 times, most recently from 957259f to e4bdaed Compare May 5, 2024 06:07
rbeurskens
rbeurskens suggested changes May 5, 2024
View reviewed changes
src/NSubstitute/Core/CallSpecification.cs Outdated Show resolved Hide resolved
src/NSubstitute/Core/CallSpecification.cs Outdated Show resolved Hide resolved
src/NSubstitute/Core/Arguments/ArgumentMatcher.cs Outdated Show resolved Hide resolved
src/NSubstitute/Core/Arguments/ArgumentMatcher.cs Outdated Show resolved Hide resolved
@dtchepak dtchepak requested a review from rbeurskens May 5, 2024 12:37
@dtchepak dtchepak force-pushed the improve-output-for-expected-arg-match branch from aa3cd12 to 50401f7 Compare November 10, 2024 03:55
@dtchepak
Improve output for expected argument matchers
90d93ec 
- Add IDescribeSpecification to allow custom arg matchers to provide
  custom output for "expected to receive" entries.
- Fallback to ToString when IDescribeSpecification not implemented.
- Update code comment docs accordingly.

Relates to nsubstitute#796.
@dtchepak
Apply review comments
72005d0 
- use string.Empty for null value from IDescribeSpecification, rather than
  falling back to ToString(). This supports the contract that
  IDescribeSpecification will be used if implemented. Replacing
  null string.Empty with matches the documented
  behaviour of IDescribeNonMatches.
- updated IDescribeSpecification code docs.
- removed GenericToNonGenericMatcherProxyWithDescribe `ToString` as it
  can use the GenericToNonGenericMatcherProxy superclass implementation.
- update ArgumentSpecification to also support IDescribeSpecification
  for its matcher.
- Replace linq with Array.ConvertAll rather than requiring an extra
  ToArray conversion.
@dtchepak dtchepak force-pushed the improve-output-for-expected-arg-match branch from 50401f7 to 72005d0 Compare November 17, 2024 03:59
@dtchepak dtchepak requested a review from Romfos November 17, 2024 04:00
zvirja
zvirja approved these changes Nov 18, 2024
View reviewed changes
Copy link
Contributor

@zvirja zvirja left a comment

Choose a reason for hiding this comment

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

Looks good to me! Small feature, but could be a good quality of life enhancement

@Romfos
Copy link
Contributor

Romfos commented Nov 18, 2024 •
edited
Loading

One question why do we need ?? string.Empty? for me it looks like anti-pattern

@rbeurskens
Copy link

One question why do we need ?? string.Empty? for me it looks like anti-pattern

Yes, I agree it feels like smell. IDK if it is needed, but especially since nullable reference types is enabled in the code and the specified methods are declared to return string (not string?) we could trust the implementation does not return null, or replace ?? string.Empty with ?? throw new NullReferenceException()

@dtchepak dtchepak merged commit 116db20 into nsubstitute:main Nov 30, 2024
6 checks passed
@dtchepak
Copy link
Member Author

One question why do we need ?? string.Empty? for me it looks like anti-pattern

Yes, I agree it feels like smell. IDK if it is needed, but especially since nullable reference types is enabled in the code and the specified methods are declared to return string (not string?) we could trust the implementation does not return null, or replace ?? string.Empty with ?? throw new NullReferenceException()

I've merged as-is as the PR is pretty old already, but would be very happy to accept a PR to improve this code. 🙇

@dtchepak dtchepak deleted the improve-output-for-expected-arg-match branch November 30, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbeurskens rbeurskens rbeurskens approved these changes

@zvirja zvirja zvirja approved these changes

@alexandrnikitin alexandrnikitin Awaiting requested review from alexandrnikitin

@Romfos Romfos Awaiting requested review from Romfos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve output for expected argument for custom argument matchers on non-match or make it customizable
4 participants
@dtchepak @Romfos @rbeurskens @zvirja