search

robinvanderknaap / MvcJqGrid

Fluent jqGrid Html Helper for ASP.NET MVC
www.playground.webpirates.nl/mvcjqgrid
GNU Lesser General Public License v3.0
101 stars 74 forks source link

Added multiple search options for Column. #36

Closed ciprianpopask closed 10 years ago

ciprianpopask commented 10 years ago

Have used Flags enum to allow setting the search options all-at-once.

robinvanderknaap commented 10 years ago

Wouldn't it be easier to use a params object to set multiple searchoptions? Now you have to set multiple searchoptions like this:

SetSearchOption(SearchOptions.BeginsWith | SearchOptions.DoesNotContain).

Intellisense does not provide a clue you can add multiple searchoptions. If we use params (SetSearchOption(params SearchOptions[] searchOptions) the developer would have a clue what to do, and it would look like this:

SetSearchOption(SearchOption.BeginsWith, SearchOptions.DoesNotContain)

Can you shed some light on why you choose to use the first syntax? Am I overlooking something?

robinvanderknaap commented 10 years ago

Your changes are included in NuGet package 1.0.13

ciprianpopask commented 10 years ago

The reason I chose this syntax was because I viewed the options as "flags" behavior-wise: being able to set multiple values at once, all passed within a single value, to specify parameters for an operation. So I went following the common approach for working with bit-flag enums. It is not a case of performance here, just a design choice based on my perspective on the searchoptions.

I see your point though. For instance, it would have made more sense to also rename the enum to "SearchFlags" to give a proper hint to the developer on how to use it.

I like your alternative solution as well. If you favor it over the "flags" approach, you can choose this one as final implementation, it's a good fit.