Expose Top or Take

[ardalis]

Expose Top or Take

[fiseni]

This is implemented. If no one has additional comments, we may close the issue.

[fiseni]

Hey @ovation22 ,

If I understood correctly, you want to expose Take in the specification? You want to use Query.Take(4), instead of Query.Paginate(0, 4) ?

What would be the benefit actually? Not writing the 0?
Implementation wise, not hard to add. But, we have to ensure that Paginate() and Take() won't be used within the same specification, since it will overwrite the values. And that might add to the confusion.

[ardalis]

It's a good point about the two being exclusive of one another. @ovation22 and I were discussing this Friday and my first thought was that it would be more intuitive to offer a .Take directly rather than having to "page" to do it. Conceptually to me at least, I don't think about paging as a way to get "Top X Rows/Records". Under the covers I know paging uses Skip/Take but it still seems like a bit of a hack to have to go through the Paging abstraction just to take advantage of this fact.

It shouldn't be too hard for us to support Take or Paginate, and throw if both are used, right?

[fiseni]

Not a problem to implement. I was just thinking how to provide a consistent API. Not a huge fan of overlapping functionalities :)
If we think twice, Paginate is just a dummy wrapper and made sense in the legacy API, but now that we have LINQ alike structure, we may be more verbose and expose Top and Skip directly. For now we can support both ways, and decorate the Paginate as obsolete. In the future we'll remove it completely. Makes sense?

But, I have a feeling we misunderstood @ovation22. He might want to get top records regardless of the specification (for any spec). If this is a case, this can be done in your repository implementation.
We provide RepositoryBase generic repository ready to use. It's an abstract one, so anyway you will have to derive from it. You could do something like this in your repo.

public interface IRepository<T> : IRepositoryBase<T> where T : class
{
    Task<List<T>> GetTopAsync(ISpecification<T> specification, int take);
    Task<List<TResult>> GetTopAsync<TResult>(ISpecification<T, TResult> specification, int take);
}

public class Repository<T> : RepositoryBase<T>, IRepository<T> where T : class
{
    protected readonly TestDbContext dbContext;

    public Repository(TestDbContext dbContext) : base(dbContext)
    {
        this.dbContext = dbContext;
    }

    public async Task<List<T>> GetTopAsync(ISpecification<T> specification, int take)
    {
        return await ApplySpecification(specification).Take(take).ToListAsync();
    }

    public async Task<List<TResult>> GetTopAsync<TResult>(ISpecification<T, TResult> specification, int take)
    {
        return await ApplySpecification(specification).Take(take).ToListAsync();
    }
}

PS. I just noticed ApplySpecification is a private method. We'll make that protected, no harm of exposing it.

[ovation22]

I was specifically referencing using Top/Take in a Specification, but I appreciate the option with the IRepository.

Giving it more thought over the weekend I was thinking along those lines same lines as you mention @fiseni if it would make sense to expose Take and Skip and deprecating Paginate. Makes sense to me.

[ardalis]

One of the key benefits of the Specification pattern IMO is that it helps prevent the need for repository implementations to have tons of different methods. If we can avoid adding more methods to repository by allowing fewer methods to do the same thing using a specification, I think we should do that. I also don't want to require devs to have to use our base repository just because they want to use specifications (as that will hurt adoption).

So, I think your original idea works:

Not a problem to implement. I was just thinking how to provide a consistent API. Not a huge fan of overlapping functionalities :)
If we think twice, Paginate is just a dummy wrapper and made sense in the legacy API, but now that we have LINQ alike structure, we may be more verbose and expose Top and Skip directly. For now we can support both ways, and decorate the Paginate as obsolete. In the future we'll remove it completely. Makes sense?

I'm still not sure I want to drop Paginate, since I like the naming of it (it's a higher level abstraction that covers up the low level skip/take details) but I agree it's not great design to have 2 ways to do the same thing.

[ardalis]

Giving it more thought over the weekend I was thinking along those lines same lines as you mention @fiseni if it would make sense to expose Take and Skip and deprecating Paginate. Makes sense to me.

Sounds like we all agree. Add Take/Skip to specification and mark Paginate obsolete.

[fiseni]

I do share your opinion regarding the repositories. I didn't mean to provide the functionality through the repo, or enforce it in any way. It's just an example how users can implement such thing in their code base, in their repo (cos that's the only way you can get such general Top functionality).

I'm free for the next 30 mins, I'll do the changes now.

[ardalis]

Ok so all I need to do if it looks good is publish a new point release, yes?

[fiseni]

Yes, I think so.
I don't think we have some breaking changes, but you might want to go through the history and cross check. There is a comment on the last commit too.

Other than that, tests should be refactored. It's in my backlog too, if I find time in the next period, I'll try to go through that.