herbsjs / herbs2knex

Create repositories using Knex to retrieve and store Entities
Other
7 stars 13 forks source link

Implementation of findAll #11

Closed maikvortx closed 3 years ago

maikvortx commented 3 years ago

Hi!

I think findAll should be like findBy, but changing to return all records, correct?

I'm implementing this solution for getAll():

async findAll() {

    const tableFields = this.dataMapper.tableFields()    

    const ret = await this.runner
      .select(tableFields);      

    const entities = []

    for (const row of ret) {
      if (row === undefined) continue
      entities.push(this.dataMapper.toEntity(row))
    }

    return entities
  }
jhomarolo commented 3 years ago

I think this functionality is very good, but I think it is valid to come with the limit option included.

maikvortx commented 3 years ago

Hi @jhomarolo !

Something like this:

async findAll({ limit: 0, orderBy: null }) {

    const tableFields = this.dataMapper.tableFields()    

    const ret = await this.runner
      .select(tableFields)
      .limit(limit);      

    const entities = []

    for (const row of ret) {
      if (row === undefined) continue
      entities.push(this.dataMapper.toEntity(row))
    }

    return entities
  }
jhomarolo commented 3 years ago

@maikvortx yes, like this. Could you please, add this into your PR?

dalssoft commented 3 years ago

if you add offset as well it would be ready to paginate (offset + limit)

maikvortx commented 3 years ago

Hello @dalssoft! I like the idea of ​​offset. I will implement these solutions and return for validation.

dalssoft commented 3 years ago

Cool. For a future PR it would be nice to implement similar options ({ orderBy, limit, offset }) on findBy

maikvortx commented 3 years ago

Ok!

maikvortx commented 3 years ago

Hi @jhomarolo ! PR updated with limit paramter.

https://github.com/herbsjs/herbs2knex/pull/13

maikvortx commented 3 years ago

@dalssoft, the findBy method can be included in findAll because the only change is searchTerm. What do you think about?

maikvortx commented 3 years ago

@dalssoft, i included find method and your wrapersfindBy and findAll. You can check? Later i will create another PR with query with pages and other functions.

dalssoft commented 3 years ago

Can we close this issue after PR https://github.com/herbsjs/herbs2knex/pull/13?