Dzoukr / Dapper.FSharp

Lightweight F# extension for StackOverflow Dapper with support for MSSQL, MySQL, PostgreSQL, and SQLite
MIT License
374 stars 35 forks source link

Linq builders #26

Closed JordanMarr closed 3 years ago

JordanMarr commented 3 years ago

INCOMING BIG PR!!

Includes:

Whew... that was a lot of work, but definitely worth it, I think! Your API is beautifully written and was a joy to interact with. In fact it was so good that I didn't have to touch it at all... except one small thing: I had to add a new case to your Where union: Expr of string. This made it possible for me to do Column to Column comparisons in the where statements. It was either that or create a ColumnToColumn case, but I opted not to do that because it would have involved creating a new comparison union (Eq, Gt, etc), which would have clashed with the existing Column to Value comparison union. So the Where.Expr allowed me to easily build the left and right side columns along with the comparison operators (plus some may find it handy to have in general for edge cases).

I paid a lot of attention to your very organized coding conventions, and I think it fits in very well.

Regarding the readme, I really wasn't sure how you would want to incorporate the new stuff, so I just copied most of what you had and it put it down the bottom. Of course that is just a starting point, so feel free to change it as you see fit!

I have tested quite a bit on SQL Server, but I haven't test it with the other two database providers. My hope was that since the LINQ Provider sits atop and feeds into the base API, that everything should hopefully just work. (haha! we'll see). I'm sure I will be fielding some bugs as people plug all kinds of crazy stuff into the provider. But I think it should be pretty solid for 99% of the standard stuff.

Maybe the best thing to do would be to push this as an "preview build" on NuGet so that people can kick the tires a bit?

Dzoukr commented 3 years ago

Wow, man, that's amazing!!! I need to admit when you wrote on Twitter you are planning something, I had to have a look. πŸ˜„ But still... this will πŸš€ the library!!!

Maybe one remark: Why don't we have table<Person> instead of entity<Person> ? Is there any reason I cannot see right now? Then you would have nicely written (and read) code like:

insert {
    into table<Person>
    value newPerson
}

Anyway... this is amazing and I think we should make it as a new major version v2.0.0 (and be ready for further bugfixing, if something happens)

JordanMarr commented 3 years ago

I am fine with β€˜table’ over β€˜entity’! That probably would be better.

oh one other thing, I had to change the .net version in the config file for the version I have installed and I forgot to change it back.

Dzoukr commented 3 years ago

No problem at all! Should I do the entity -> table change or you'll do it? If you would not mind, I'd prefer you'd do it - you know all the places where it's used. I'll merge PR then, bump up the version and do the announcement?

JordanMarr commented 3 years ago

Sure, I will change it.

JordanMarr commented 3 years ago

Done... so now there is table, mapTable and mapSchema. Not sure if the last two should also be renamed... πŸ€”

Dzoukr commented 3 years ago

Hmm... Good question. πŸ€”

Maybe namedAs and namedSchemaAs ? Or aliasFor and aliasForSchema Don't know really. 🀷 πŸ˜„

I feel the map is not the best naming here, but it's not a big deal neither. Naming is hard. πŸ˜„

JordanMarr commented 3 years ago

Another thing I went back and forth about was that update set takes any type (to allow for partials). I wondered if I should constrain set to be the same type as the table, and then have a setPartial that gives more freedom to allow for any type. Maybe that would be a little more explicit for using other types. But then again, I could just be overthinking things, and it's probably fine the way it is.

Dzoukr commented 3 years ago

I think the set is ok as it is. Even with your amazing contribution to more type safety, developers still need to think about what they are doing, so probably more freedom is good here.

Dzoukr commented 3 years ago

Also... Having set with constraint... It would mean that you'd always do the full update (of the whole row), right?

JordanMarr commented 3 years ago

Also... Having set with constraint... It would mean that you'd always do the full update (of the whole row), right?

My thinking was that you could choose either set, which would have to match the table type exactly, or another setPartial (or something like that) which could be an anonymous record or different record.

Dzoukr commented 3 years ago

I think it's good as it is right now. So can I merge the PR?

Dzoukr commented 3 years ago

Let me know when you feel it's good to merge, I can't wait to announce it (also good to know your Twitter handle to give you all the credit for your amazing work πŸ’ͺ)

JordanMarr commented 3 years ago

Hold on, I think I've got a great idea for the table name thing. Let me try this out first and then I think we'll be good!

Dzoukr commented 3 years ago

Sure, I'll just wait for your 🏁

JordanMarr commented 3 years ago

After many attempts, all of which seemed too verbose, I think I've hit naming gold by setting the name in one shot!

Table Mappings

You can either specify your tables within the query, or you can specify them above your queries (which is recommended since it makes them sharable between your queries). The following will assume that the table name exactly matches the record name, "Person":

let personTable = table<Person>

If your record maps to a table with a different name:

let personTable = table'<Person> "People"

If you want to include a schema name:

let personTable = table'<Person> "People" |> inSchema "dbo"
JordanMarr commented 3 years ago

I say merge and announce when ready! 🎈(●'β—‘'●)🍿

Dzoukr commented 3 years ago

It's too late - this needs a prime time!! 😁 So I'll wait for tomorrow morning (I'm in horizontal position now anyway).

I love how you resolved the naming! Not a big fan of ' sign but it's way shorter than tableAlias<Person> "People" or tableName<Person> "People" so lets keep it like that. Do you have a Twitter I can pass through all the community kudos? 😁

JordanMarr commented 3 years ago

Good call on the prime time slot!

Yes, I've never been a fan of the apostrophe in names; in fact, this is the first time I've ever used one! But it just seemed to make sense in this very specific situation.

My Twitter Profile

tforkmann commented 3 years ago

This is actually amazing!! Combined with Facil it’s like a typeprovider experience. Thank you guys!