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

Use nameof instead of strongly typed string #1896

Merged
merged 4 commits into from
Jul 18, 2019

Conversation

yahiheb
Copy link
Collaborator

@yahiheb yahiheb commented Jul 12, 2019

No description provided.

Copy link
Contributor

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

($"Could not parse {nameof(Height)}.");
prototype to nameof(Prototype).ToLower()

@nopara73
Copy link
Contributor

Concept ACK. Can you fix the conflict before review?

@yahiheb
Copy link
Collaborator Author

yahiheb commented Jul 13, 2019

@nopara73 Conflicts fixed.

@@ -300,7 +298,7 @@ private OptionValueType ParsePrototype()
string name = Names[i];
if (name.Length == 0)
{
throw new ArgumentException("Empty option names are not supported.", "prototype");
throw new ArgumentException("Empty option names are not supported.", nameof(Prototype).ToLower());
Copy link
Contributor

Choose a reason for hiding this comment

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

Bacause ArgumentException second parameter is the exact name of param, IMO .ToLower() is unnecessary even if the original Exception worked like that. These texts are written to the user on the console so changing it won't cause any trouble.

Copy link
Collaborator Author

@yahiheb yahiheb Jul 17, 2019

Choose a reason for hiding this comment

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

In other exception messages "prototype" all lower case was used, that is the only reason I used .ToLower(). I can remove it if you want.

Copy link
Contributor

@molnard molnard Jul 17, 2019

Choose a reason for hiding this comment

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

@nopara73 Should we stay with prototype with lower case in ArgumentException? The name of the property is Prototype so that would be the correct one but in the original code it is prototype.
These messages are written to the console to the user.

WalletWasabi/Mono/OptionSet.cs Outdated Show resolved Hide resolved
WalletWasabi/Mono/OptionSet.cs Outdated Show resolved Hide resolved
@nopara73 nopara73 merged commit 8d5cf49 into WalletWasabi:master Jul 18, 2019
@yahiheb yahiheb deleted the nameof-interpolation branch July 18, 2019 13:16
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.

4 participants