mrahhal / MR.EntityFrameworkCore.KeysetPagination

Keyset/Seek/Cursor pagination for Entity Framework Core.
MIT License
218 stars 11 forks source link

Expose KeysetPaginationBuilder.ConfigureColumn #50

Closed dheardal closed 5 months ago

dheardal commented 8 months ago

Can I change the access modifier on KeysetPaginationBuilder.ConfigureColumn to public?

At the moment I am having to write twice the amount of code required to use the fluent methods ascending/descending which I wouldn't need to do if I could just pass in true/false for descending

If not could i create a new method that takes in System.ComponentModel.ListSortDirection ?

mrahhal commented 8 months ago

There's no big reason for ConfigureColumn to be private, but since right now it's not, you can always add your own extension method that replicates it. I'm hesitant about making it public because in the vast majority of cases you won't need it, and I would like to keep it hidden as the boolean argument can be opaque (and not sure about adding a direction enum just for this, at least for now).

Can you show your case (a code snippet) of why you're having to do this? That would help me form a better opinion.

dheardal commented 8 months ago

I ended up doing as you say and writing an extension method around it.

I disagree that in the vast majority of cases you wouldn't use it, since order direction is usually passed through as an API parameter most implementations of this would need to write an extension method to call the right method which is kind of annoying given there is a perfectly good method already!

mrahhal commented 8 months ago

If you have a dynamic direction on each request, this still wouldn't be used if you're using prebuilt definitions (which is the recommended way). If you're not prebuilding the definitions, then yes I assume in this particular case you would benefit from having one method with an arg for the direction. That's why a code snippet from your use case always helps.

I might need to convert the direction into an enum before making it public since a boolean here would be too opaque. So I'll hold off on the PR until I do that (unless if you want to attempt and do this in the PR, adding a new KeysetColumnDirection enum, and processing the changes in the code), I'll keep this issue open.

dheardal commented 8 months ago

For reference this is the extension method I built:

    public static KeysetQueryDefinition<TEntity> GetPagingQueryDefinition<TEntity, T>(
        this PagingRequest pagingRequest,
        Expression<Func<TEntity, T>> columnAccessor,
        ListSortDirection defaultOrderDirection = ListSortDirection.Ascending)
        where T : IComparable, IComparable<T>
    {
        return (pagingRequest.OrderDirection ?? defaultOrderDirection) == ListSortDirection.Ascending ?
            KeysetQuery.Build<TEntity>(b => b.Ascending(columnAccessor)) :
            KeysetQuery.Build<TEntity>(b => b.Descending(columnAccessor));
    }

Quite hard to use prebuilt definitions for user defined requests, even if it is recommended :)

mrahhal commented 8 months ago

Unless I'm mistaken, this is using the prebuilt definitions, and would benefit with having one method with an arg:

You're creating a new definition each time you call this method, so this isn't using prebuilt definitions correctly. I'm guessing you're calling this method in the action? As I mention in the README, you should put the result of KeysetQuery.Build into a long lived instance to use it properly.

In any case, it's true that it's not always feasible to use a prebuilt definition.

mrahhal commented 5 months ago

Release in v1.4.0.