Skip to content

Use the C# identifier from the calling code in the failure message instead of a generic name #705

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
Merged
dennisdoomen merged 3 commits into from
Jan 6, 2018
Merged

Use the C# identifier from the calling code in the failure message instead of a generic name #705

dennisdoomen merged 3 commits into from
Jan 6, 2018

Conversation

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Dec 9, 2017 •
edited
Loading

See #588

Order of precedence

  1. Context (for nested object graph assertion)
  2. Identifier extracted from the calling source code
  3. Fallback identifier specified in the {identifier:fallback} placeholder
  4. Generic fallback provided through WithDefaultIdentifier

@dennisdoomen dennisdoomen changed the title Use the C# identifier from the calling code in the failure message instead of a generic name [WIP] Use the C# identifier from the calling code in the failure message instead of a generic name Dec 9, 2017
@dennisdoomen dennisdoomen force-pushed the CallerContext branch 5 times, most recently from d65cb78 to 0ebf088 Compare December 15, 2017 20:25
jnyrup
jnyrup reviewed Dec 15, 2017
View reviewed changes
Src/FluentAssertions/CallerIdentifier.cs Outdated

private static bool UsesNewKeyword(string candidate)
{
return new Regex(@"new(\s?\[|\s?\{|\s\w+)").IsMatch(candidate);
Copy link
Member

@jnyrup jnyrup Dec 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Just nitpicking, but you could use the static Regex.IsMatch which caches the regex.

Copy link
Member Author

@dennisdoomen dennisdoomen Dec 16, 2017

Choose a reason for hiding this comment

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

Good point. I still have to clean up the code. Currently focusing on getting all tests green.

@dennisdoomen dennisdoomen force-pushed the CallerContext branch 3 times, most recently from a243fe1 to 1ffa97d Compare December 26, 2017 06:02
@dennisdoomen dennisdoomen mentioned this pull request Dec 26, 2017
Merged
@dennisdoomen dennisdoomen force-pushed the CallerContext branch 4 times, most recently from 66d37df to 3f76d50 Compare December 29, 2017 20:23
@dennisdoomen dennisdoomen mentioned this pull request Jan 2, 2018
Closed
@dennisdoomen dennisdoomen force-pushed the CallerContext branch 6 times, most recently from 82da703 to f023f57 Compare January 5, 2018 14:46
@dennisdoomen dennisdoomen force-pushed the CallerContext branch 2 times, most recently from eb5c70a to 33c1255 Compare January 6, 2018 06:24
@dennisdoomen dennisdoomen changed the title [WIP] Use the C# identifier from the calling code in the failure message instead of a generic name Use the C# identifier from the calling code in the failure message instead of a generic name Jan 6, 2018
@dennisdoomen dennisdoomen merged commit fe0af34 into fluentassertions:release-5.0 Jan 6, 2018
@dennisdoomen dennisdoomen deleted the CallerContext branch January 6, 2018 11:23
@jnyrup
Copy link
Member

jnyrup commented Jan 7, 2018

Is CallerIdentifier.logger intentional or an just oversight before committing?

@dennisdoomen
Copy link
Member Author

dennisdoomen commented Jan 7, 2018

I used it for diagnostic purposes and plan to keep it in in case it misbehaves. Have to think of a way to make it available as part of the tracing functionality of the AssertionScope.

@jnyrup
Copy link
Member

jnyrup commented Jan 7, 2018

Ok.
I mostly just noticed that there was a public static lowercase field in a public class, which was never reassigned.

@dennisdoomen
Copy link
Member Author

dennisdoomen commented Jan 7, 2018

Yeah, thanks for reminding me about that one.

@jnyrup jnyrup mentioned this pull request Feb 5, 2018
Merged
@dennisdoomen dennisdoomen mentioned this pull request Jan 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnyrup jnyrup jnyrup left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dennisdoomen @jnyrup