-
Notifications
You must be signed in to change notification settings - Fork 649
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
Migration.Insert.Row where a value is Enum #1441
Comments
Would it work if you cast the value? public enum myEnum
{
Value1 = 12334
}
var values = new Dictionary<string, object>
{
{ "intColumn", (int)myEnum.Value1 }
};
Insert.IntoTable("myTable")
.Row(values); |
@jzabroski, thank for the reply. |
@MihailsKuzmins There is no enum handling . The code does not do any coercion. If you find it unnatural, I would suggest creating a wrapper syntax, where you traverse the dictionary and update object values from enum values to integral storage values. However, you might want to also handle the underlying storage of the enum type, when you perform your cast. public enum myEnum : short
{
Value1 = 12
} I would think of this transform as similar to using Linq and calling Cast on an Array. |
If you want to submit a PR for handling enums, you can, but you need to handle all possible integral storage types in your unit tests. Thank you, |
@jzabroski So, due to the fact that I don't see a good solution, could you please advise, what could be a better approach? One of the aforecited or a completely different one? Thanks |
These are always hard guidance to give. Sometimes, I make mistakes, even with 20 years c# experience. Sometimes, you just have to play with it to see what feels right. I did have an idea for creating a "FluentMigrator.Opinions" nuget package (see #1206) which would allow people to export syntax that matches my aesthetic tastes. Since FluentMigrator is over 12 years old, it has the accumulation of many people's tastes over time. As an example of tastes, some people might want to store enums in the database by their That said, I would break the problem down into the following, testable pieces:
|
When you mentioned the "pickling method" (serialisation, right?) it gave me an idea to look at But looking at the bullet points 2 and 3, unfortunately, again I'm a bit lost.
So you suggest that there should be something like options which determine how to handle enum values? For example in
The |
@MihailsKuzmins Sorry I didn't reply back - the day of your last reply, I was sick with COVID-19. Are you still interested in working on this? |
@jzabroski oh, I'm sorry. I hope you are doing well now. |
Cool - let's re-open this. Yeah, I have time (I am just now getting close to 100% strength even though I have been covid negative for like a month). Hoping to take a look at the backlog this weekend and catch up. I think everything you brought up makes sense, but I would need to dive into the code to give exact guidance. You brought up a good point about the pickler needing to handle either numbers or strings. |
@jzabroski, I've decided to stop talking and actually write some code, so that we have something to discuss. |
Most of those PRs are dependabot or more discussion-based PRs... only two or three are real, and one is waiting on someone to refactor it... I will review your PR, either today or tomorrow. @fubar-coder if you would like to review as well. |
@jzabroski The PR is a good first start. This is what came to my mind:
|
@fubar-coder, thank you for the review
|
First, I'd like to thank you for your work and the effort to improve FM. Regarding 2: IIRC the injection of quoters, type maps, etc... has some quirks when it comes to using custom implementations and support for multiple databases in the same configuration. @jzabroski Maybe we should drop support for multiple databases in the same service collection and should just add the FM services required for a specific database, based on the current configuration? Regarding 3: I think that inheritance might be the better choice here, but we can only implement this change if we're dropping all .NET Core versions < 3.0. This is for @jzabroski to decide. Regarding 4: I understand your point, but this should be decided by @jzabroski too. |
I reviewed this - I am commenting on 4 as a xUnit.net person who dislikes NUnit. Personally, I think There's really no right or wrong way to do this - as long as we're consistent. Most of the drawbacks come from the fact that when you build a set of "facts" about your code you're effectively arranging a lattice of judgments about whether some set of truths is accurate/truly true. Introducing any negation is going to break that lattice and cause refactoring. |
Separately, I pointed out it seems like we're breaking the ABI for FirebirdGenerator by not leaving the old constructor around and marking it as obsolete. I think that we should not allow three-valued logic for the QuoterOptions - it should be not nullable. Regarding Microsoft DI FirebirdQuoter - the way Microsoft DI works, you can get in trouble if it's not deterministic how to construct the object, and so you end up using a lambda to register it - but when you do that, I recall the "messy" part is things can "fall through" unexpectedly - I can't remember the exact scenario but lambdas can cause problems based on the order in which you ask for things, I think. Is that what you're referring to? In other words, it would be possible to do a Lambda that registers based on some key passed into the DI extension method. But then people forget to call the extension method to get the right type, so you end up with a lot of support requests with people not reading the docs/code well. Often, it's because there is some blog post or gist out there with a snippet of "how to do things" that isn't kept up to date. |
Ok, it seems my explanation wasn't clear enough, so I hacked together this sample code: var services = new ServiceCollection();
// In user code:
services
.Configure<QuoterOptions>(s => s.EnumAsUnderlyingType = true)
.Configure<FirebirdOptions>(s => s.TruncateLongNames = true);
// Inside the AddFirebird extension method:
services
.AddOptions<FirebirdOptions>()
.Configure<IOptions<QuoterOptions>>((fo, qo) =>
{
// Maybe using AutoMapper or something would be useful here to
// be sure that the values of all properties are copied from QuoterOptions
// to FirebirdOptions.
fo.EnumAsUnderlyingType ??= qo.Value.EnumAsUnderlyingType;
});
var serviceProvider = services.BuildServiceProvider();
var firebirdOptions = serviceProvider.GetRequiredService<IOptions<FirebirdOptions>>();
// Both EnumAsUnderlyingType and TruncateLongNames are set to "true" here.
firebirdOptions.Dump();
class QuoterOptions
{
// "null" will be interpreted as "false"
public bool? EnumAsUnderlyingType { get; set; }
}
class FirebirdOptions : QuoterOptions
{
public bool TruncateLongNames { get; set; }
/* ... and more of those */
} This code change allows to set We could also keep the services
.AddOptions<FirebirdOptions>()
.Configure<IOptions<QuoterOptions>>((fo, qo) =>
{
// Maybe using AutoMapper or something would be useful here to
// be sure that the values of all properties are copied from QuoterOptions
// to FirebirdOptions.
fo._enumAsUnderlyingType ??= qo.Value._enumAsUnderlyingType;
});
class QuoterOptions
{
internal bool? _enumAsUnderlyingType;
public bool EnumAsUnderlyingType
{
get => _enumAsUnderlyingType ?? false;
set => _enumAsUnderlyingType = value;
}
} With an This would allow us to configure "global" quoter options in However, I'm not entirely sure that this would be the right choice here, as it would also limit us to the Microsoft.Extensions.* libraries of .NET Core 3 and it might be overly complicated and error-prone. |
@jzabroski, @fubar-coder thank you for the review!
For know, I guess, there are two open questions:
|
I haven't found a similar issue, but please redirect me if it actually exists. (I apologise if I've created a duplicate by accident)
I have a problem with this code
The error I get: !!! Incorrect integer value: 'Value' for column 'intColumn' at row 1
So it means that the runner transforms enums into their string values instead of their underlying numeric type which is not the right behaviour.
Would it be possible to take a look at this case? I can possibly try to do it as well
The text was updated successfully, but these errors were encountered: