Skip to content

Improve output for expected argument matchers#806

Merged
dtchepak merged 2 commits intonsubstitute:mainfrom
dtchepak:improve-output-for-expected-arg-match
Nov 30, 2024
Merged

Improve output for expected argument matchers#806
dtchepak merged 2 commits intonsubstitute:mainfrom
dtchepak:improve-output-for-expected-arg-match

Conversation

@dtchepak
Copy link
Copy Markdown
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 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
- 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.
- 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
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Romfos commented Nov 18, 2024

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

@rbeurskens
Copy link
Copy Markdown
Contributor

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
@dtchepak
Copy link
Copy Markdown
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. 🙇

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.

Improve output for expected argument for custom argument matchers on non-match or make it customizable

4 participants