Add AllParametersOption as safer alternative to AllParametersOptional

[cmeeren]

Using AllParametersOptional is more dangerous than it needs to be. Since it makes all parameters optional, you get no compile-time check when you add new parameters to your SQL and forget to add them to Execute/AsyncExecute.

A safer alternative would be to make the parameters option-wrapped instead of implementing them as optional parameters. That way, all parameters would still be mandatory, and you'll get a compiler error if you add new params to your SQL without adding them to Execute/AsyncExecute.

I therefore suggest adding a new option AllParameterOption (in addition to the existing AllParametersOptional), so that users can choose a safer alternative if they want.

(Feel free to choose a better name for the static parameter, of course.)

[cmeeren]

Case in point: I just made a big blooper that could have been prevented with this feature - I added more params to the SQL, but forgot to add them to AsyncExecute because I have AllParametersOptional. They were then null in the SQL, and some subtly erroneous behaviour was pushed to production.

Make no mistake, it's entirely my own fault - but if SqlClient had the AllParametersOption that I describe above, this would have been a non-issue, since it wouldn't compile until I had added the new parameters to AsyncExecute. :)

[smoothdeveloper]

Thanks for reporting this @cmeeren, I wasn't aware that AllParametersOptional was also marking the parameters as optional in F# sense, I thought it only changes the type of the parameters to be Option / to map nullable parameters.

I'd like to consider if removing that aspect, and just keep the option wrapping would hurt the users.

That aspect of the current feature reminds me the syntactic sugar around COM and optional parameters introduced in C# 4: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments#com-interfaces

The problem with removing the feature is if user of the library is calling procedure/query with lots of arguments and wants to keep code terse, without passing all parameters explicitly.

I think for regular usage, having to specify all parameters, and also, having to specify the argument names (https://github.com/fsharp/fslang-suggestions/issues/414) is what should client code looks like.

I therefore suggest adding a new option AllParameterOption (in addition to the existing AllParametersOptional), so that users can choose a safer alternative if they want.

I'm not really keen myself on adding extra parameters:

• documenting the nuance with existing one (even if we distinguish the names more)
• the confusion it will create
• they don't necessarily compose well, especially if we add them without considering carefully the alternatives

My current preference would be to get rid of the optional parameter attributes that currently gets added on the methods, and require all callsites to have their parameters defined.

Right now, I feel it trumps some of the type safety / maintainability concerns that are core to the design of this provider.

Let's gather more feedback from users and maintainers of the library.

[cmeeren]

My current preference would be to get rid of the optional parameter attributes that currently gets added on the methods, and require all callsites to have their parameters defined.

Right now, I feel it trumps some of the type safety / maintainability concerns that are core to the design of this provider.

I agree, on both accounts.

As for adding a new parameter: Yes, they don't compose well, so if you end up doing that, you might want to require at most one of them being specified (returning an error if both are supplied).

Let's gather more feedback from users and maintainers of the library.

Sounds wise. How? Will you reach out to people, or will we be silently waiting for someone to happen upon this issue and voice their opinion? (No-one has for the last two months, so, that's probably not very effective.)

[smoothdeveloper]

you might want to require at most one of them being specified (returning an error if both are supplied)

This is a great idea if we go with another parameter, the current parameter could be marked obsolete and/or renamed

In case users of the library prefer deprecation, my proposal would be:

• rename current parameter to MakeMethodParametersOptional
• if 0 voice is against it, mark it obsolete, with a message guiding developer about potential issues using this and how to add pragma or fix the call sites, this message should show as a warning
• use the current name for the behaviour you and I have been describing, I think this is the default behaviour users would expect

In case we just replace the behaviour, there is less work to get this issue addressed.

Sounds wise. How?

I've been contributing to the library but I'm by no mean in place to take ownership of the API design, and I'm interested in keeping ideas coming from all sides to figure out a good solution to any given issue as this is OSS and previous point.

Will you reach out to people, or will we be silently waiting for someone to happen upon this issue and voice their opinion?

I'm reaching out by different means, and even if silently waiting :)

I don't currently have time beside triaging a bit the tickets and reviewing PRs once in a while, unless it is something I do need for my day work.

I'm concerned about the number of open issues, new features aren't necessarily a priority compared to other efforts that can be expanded on this library at large, at least on my spare time.

If some "ok for breaking change" feedback comes, the amount of work to get the library where you want will be less than having to support both concepts of optional parameters in support, documentation, tests, etc.

my plan:

• feedback on breaking change versus deprecation path welcome, @sergey-tihon would it be possible to push a "request for discussion" in an upcoming F# Weekly? @fsprojects / @mathias-brandewinder possible to make a tweet?
• silently wait
• PR for this welcome
• review cycle / merge / release

[smoothdeveloper]

@cmeeren, if you need to implement the work around in the meantime:

https://github.com/fsprojects/FSharp.Data.SqlClient/blob/5cd650a5586a31109829658c0f1713cf97d5d19f/src/SqlClient.DesignTime/DesignTime.fs#L587

Could you give a try at removing , optionalValue = null on that line, I think that will do what you want.

[cmeeren]

Thanks. To keep things simple for the other folks at my company, I'll stick with the nuget version. Nice to know the change might be so simple, though.

[shawn-martin]

My 2 cents: breaking change is fine. It's always been my intent to supply all the parameters whenever I've used this feature.

[cmeeren]

Seems there is general support for this idea. Would be great to see it implemented, especially since the implementation seems to be so simple!

[cmeeren]

Just got burned by this again... 😅 Any chance to implement this? Particularly since, as mentioned above, there seems to be general support for the idea and it seems simple to implement.