-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/currency code #46
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, its ok for me. Just a small change that you can do a a test class name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove Nox from the class Name
/// </summary> | ||
public sealed class CurrencyCode3 : ValueObject<string, CurrencyCode3> | ||
{ | ||
private readonly string[] CurrencyCodes = Enum.GetNames(typeof(CurrencyCode)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put this as a private readonly static variable? Also, can we rename it using the following convention https://regusit.atlassian.net/wiki/spaces/NOX/pages/3153461266/Coding+Conventions+Styles+and+Guidelines#Private-static-fields ?
@@ -0,0 +1,53 @@ | |||
using Nox.Types.Tests.Types; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write the tests using FluentAssertions, as it was agreed before?
https://regusit.atlassian.net/wiki/spaces/NOX/pages/3153264693/Test+Projects
There was a problem hiding this 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. Added a few small comments.
Nox.Types was merged to Nox.Generator. Because of that this PR cannot be merged, so @denisvieira-dev please create a new PR for this type in Nox.Generator. |
No description provided.