Skip to content

Add builders and execute for repository #792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
raymondfeng merged 1 commit into from
Dec 12, 2017
Merged

Add builders and execute for repository #792

raymondfeng merged 1 commit into from
Dec 12, 2017

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Dec 11, 2017 •
edited
Loading

Description

The PR adds execute() and builders for our Filter/Where objects.

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide

@raymondfeng raymondfeng requested a review from bajtos as a code owner December 11, 2017 16:39
@raymondfeng raymondfeng mentioned this pull request Dec 11, 2017
Closed
2 tasks
@raymondfeng raymondfeng force-pushed the add-builders-and-execute branch 2 times, most recently from 1382122 to b586c49 Compare December 11, 2017 22:28
bajtos
bajtos suggested changes Dec 12, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, @raymondfeng! I did not review all new tests and builder methods in detail, but I don't see any major problems beyond the things pointed out below.

At high level, please check why our code coverage is decreased by your changes by -0.3%, I think your tests are missing some important use cases.

packages/repository/src/query.ts
* Sorting order for matched entities
*/
order?: Order[];
order?: string[];
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried that this change will allow people to accidentally use invalid values for order and the TypeScript compiler will not catch that problem. Can we preserve this type? Unless I am missing something?

Copy link
Contributor Author

@raymondfeng raymondfeng Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few issues around this signature:

  1. Order is defined as {[field: string]: Direction}. That means we can have {a: 'ASC', b: 'DESC'}. Do we translate it to ORDER BY A ASC, B DESC? If so, can we avoid Order[]`?

  2. string[] is not very intuitive. I use it internally for now to avoid translation to legacy juggler order format.

  3. The FilterBuilder takes Order for type safety.

Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I think I got confused about the difference between Order and Direction, as it seems to be the same thing. Maybe Ordering would be a better type name?

In the light of your comment, I am ok to keep the current implementation, but please add a comment explaining why we are using string[] type here and perhaps add a link pointing to our docs where people can learn more about valid values.

packages/repository/src/query.ts Outdated
}

/**
* A builder for Where object
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include a short example showing how to use this builder.

packages/repository/src/query.ts Outdated
}

/**
* A builder for Filter
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include a short example showing how to use this builder.

packages/repository/test/unit/repository-mixin/repository-mixin.test.ts Outdated
class AppWithRepoMixin extends RepositoryMixin(Application) {}

class NoteRepo {
class NoteRepo implements Repository<any> {
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be Repository<Note>, or at least Repository<Entity>. Thoughts?

@raymondfeng raymondfeng force-pushed the add-builders-and-execute branch from b586c49 to 23a8ac7 Compare December 12, 2017 15:47
@raymondfeng raymondfeng force-pushed the add-builders-and-execute branch 7 times, most recently from 40abe76 to 796ff03 Compare December 12, 2017 18:38
bajtos
bajtos approved these changes Dec 12, 2017
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot tell what you changed in the tests (because there is only a single commit), but since the code coverage is going up now, I am happy 😄

I found few minor things to improve before this is landed, see below.

Other than that, the patch LGTM.

packages/repository/src/query.ts Outdated
* `and`, `or`, and other operators.
*
* @example
* ```
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add ts or js after the last back-tick to enable syntax highlighting.

packages/repository/src/query.ts Outdated
* @example
* ```
* const whereBuilder = new WhereBuilder();
* const where = whereBuilder
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the indentation consistent.

packages/repository/src/query.ts Outdated
* `fields`, `order`, `where`, `limit`, `offset`, and `include`.
*
* @example
* ```
Copy link
Member

@bajtos bajtos Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add ts or js after the last back-tick to enable syntax highlighting.

@raymondfeng raymondfeng force-pushed the add-builders-and-execute branch from 796ff03 to be5aec4 Compare December 12, 2017 19:00
@raymondfeng
feat(repository): Add builders and execute()
842dea8 
Add builders with fleunt APIs to build Filter and Where objects
Add execute method to Repository interfaces
@raymondfeng raymondfeng force-pushed the add-builders-and-execute branch from be5aec4 to 842dea8 Compare December 12, 2017 19:07
@raymondfeng raymondfeng changed the title [WIP] Add builders and execute Add builders and execute for repository Dec 12, 2017
@raymondfeng raymondfeng merged commit 89eaf5f into master Dec 12, 2017
@raymondfeng raymondfeng deleted the add-builders-and-execute branch December 14, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kjdelisle kjdelisle Awaiting requested review from kjdelisle

1 more reviewer

@bajtos bajtos bajtos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@raymondfeng @bajtos