handle enum values in Quoters #1475
Conversation
There was a problem hiding this comment.
@MihailsKuzminsDG Thanks for this contribution. Most of my remarks are fairly minor - typos, adjusting application binary interface to ensure no breaking changes. It looks like you took care of one of my earlier remarks about the QuoterOptions not being three-valued logic but we still need a default constructor. Also, the FirebirdGenerator constructor appears to have an ABI breaking change - can you please restore the old constructor and mark it as Obsolete so that consumers are aware.
| public interface IQuoterOptions | ||
| { | ||
| /// <remarks> | ||
| /// If <see langword="true"/> enums are converted to a string, if <see langword="false"/> enums are converted to the underlying numeric type.<br/> |
There was a problem hiding this comment.
Small grammar. Think this should say:
If , enums are converted to a string; if , enums are converted to the underlying numeric type.
| { | ||
| /// <inheritdoc /> | ||
| /// <summary> |
There was a problem hiding this comment.
What's the behavior of inheritdoc + summary back to back? I never used that. More just wondering.
There was a problem hiding this comment.
sorry, my bad. I created an interface for it where <remarks> were specified, so the <summary> was taken from the actual class and the <remarks> were taken from the interface.
Along the way I deleted the interface and probably forgot to remove the <inferitdoc /> tag 😔
| @@ -74,74 +74,74 @@ public override string ToString() | |||
| [Test] | |||
There was a problem hiding this comment.
I think this would be a lot cleaner with an NUnit Theory, so that you can parameterize the options values and outcomes.
In other words, while it might be a bit silly to set EnumAsString in the below CanEscapeAString test, it would then be possible to test not just a string, but an enum-as-string.
Thoughts?
There was a problem hiding this comment.
Unlike xUnit, NUnit has global Theory parameters (it might become painful to have multiple Theories in one class). Do you think tests for QuoteColumnName should be extracted to another class?
I am sorry, but I don't quite follow you with regards to CanEscapeAString. The property EnumAsString (now EnumAsUnderlyingType) does not affect QuoteColumnName which is tested by CanEscapeAString
There was a problem hiding this comment.
That's the point- the theory would verify it doesn't affect QuoteColumnName. It might be needless complexity since an enum field name can't have a common escape in it - but Oracle and DB2 allow for arbitrary escapes, so it gets interesting fast.
| @@ -187,6 +186,18 @@ public void EnumIsFormattedAsString() | |||
| .ShouldBe("'Bar'"); | |||
| } | |||
|
|
|||
| [Test] | |||
| public void EnumIsFormatterAsUnterlyingType() | |||
There was a problem hiding this comment.
Typo. Please find+replace EnumIsFormatterAsUnterlyingType with EnumIsFormatterAsUnderlyingType
|
|
||
| public GenericQuoter(QuoterOptions quoterOptions = null) | ||
| public GenericQuoter([CanBeNull] QuoterOptions options) |
There was a problem hiding this comment.
Please see @fubar-coder remarks about having the default constructor still available. I would not use [CanBeNull] - instead, the default constructor should just initialize the quoter to a default value. Adding nulls will just complicate future code.
src/FluentMigrator.Runner.Firebird/Generators/Firebird/FirebirdQuoter.cs
Outdated
Show resolved
Hide resolved
src/FluentMigrator.Runner.Firebird/Generators/Firebird/FirebirdGenerator.cs
Outdated
Show resolved
Hide resolved
src/FluentMigrator.Runner.Redshift/Generators/Redshift/RedshiftDescriptionGenerator.cs
Show resolved
Hide resolved
src/FluentMigrator.Runner.SqlServer/Processors/SqlServer/SqlServerProcessorFactory.cs
Outdated
Show resolved
Hide resolved
| @@ -30,9 +30,9 @@ public class GenericQuoter : IQuoter | |||
| { | |||
| private readonly QuoterOptions _options; | |||
|
|
|||
| public GenericQuoter([CanBeNull] IOptions<QuoterOptions> options = null) | |||
| public GenericQuoter(IOptions<QuoterOptions> options) | |||
There was a problem hiding this comment.
I see you did my comments in the previous commit. Agree, using [CanBeNull] and assigning to null adds messy three-valued logic that we don't need. We just need a default constructor for GenericQuoter so that we don't break ABI with existing consumers.
There was a problem hiding this comment.
[CanBeNull] for just for me in order that I don't have billions of compilation errors when I add this new parameter. But you're right, I should've added a default obsolete constructor.
| [NotNull] IOptions<GeneratorOptions> generatorOptions) | ||
| : this(new FirebirdQuoter(fbOptions.ForceQuote), fbOptions, generatorOptions) | ||
| : this(new FirebirdQuoter(fbOptions, quoterOptions), fbOptions, quoterOptions, generatorOptions) | ||
| { | ||
| } | ||
|
|
||
| public FirebirdGenerator( |
There was a problem hiding this comment.
could you please specify what parameters the obsolete overload should consume?
FirebirdQuoter seems to be injected (QuoterOptions are injected into FirebirdQuoter)
src/FluentMigrator.Runner.Redshift/Generators/Redshift/RedshiftDescriptionGenerator.cs
Show resolved
Hide resolved
|
This PR is outdated by several years and is stale @jzabroski. Any concerns about closing it? I can ping the original author to clean it up and open a new PR if needed. Thoughts? |
@schambers well as for me this functionality is not required anymore (I had a workaround that has been working until now), so if this functionality is not required by the members of the project it is fine to close the PR. |
|
We can close it. I might reimplement it now that I'm done making type map injectable |
PR for the issue #1441.
QuoterOptionswhich is passed to all QuotersEnumAsUnderlyingType(false - stringm true - numeric). Default value: false, i.e. string (as it is now)WithQuoterConventions()inMigrationRunnerBuilderExtensions