Skip to content
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.

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

fix(cli): reorder where and body in CLI template for updateAll #1504

Merged
merged 1 commit into from Jul 13, 2018

Conversation

shimks
Copy link
Contributor

@shimks shimks commented Jul 9, 2018

Current way the call is made with updateAll in the CRUD CLI template:
return await this.<%= repositoryNameCamel %>.updateAll(where, obj);

Signature of updateAll:

  updateAll(
    data: Partial<T>,
    where?: Where,
    options?: Options,
  ): Promise<number>

The where object and the body need to be switched around

fixes #1521

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

Does it make sense to update the order of the parameters passed into the controller method? -- Switching the order of @param.query.string and @requestBody?

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM

Does it make sense to update the order of the parameters passed into the controller method? -- Switching the order of @param.query.string and @requestbody?

I would +1. In the legacy-juggler-bridge where in updateAll is optional, which also makes sense to me for the new updateAll controller method.

The current code has where as a required input, so the order doesn't seem that important to me.

@bajtos
Copy link
Member

bajtos commented Jul 10, 2018

+1 for changing the order of controller method arguments too, and also making the where argument of the controller method optional in TypeScript. The way how it is annotated is making the argument required as far as the REST layer is concerned.

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

+1 for changing order of controller method arguments and making where object optional.

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM otherwise!

@shimks shimks force-pushed the fix-cli-template branch 6 times, most recently from 6402d9c to 82794e7 Compare July 10, 2018 17:32
): Promise<number> {
return await this.<%= repositoryNameCamel %>.updateAll(where, obj);
return await this.<%= repositoryNameCamel %>.updateAll(obj, where);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the obj to be Partial<<%= modelName %>>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it should be, but from what i remember of #1179, this should incorrectly generate the schema, so it's something that should probably be avoided until it can be fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It's probably time to introduce a @type decorator.

@requestBody() obj: Todo,
@param.query.string('where') where?: Where,
): Promise<number> {
return await this.todoRepository.updateAll(where, obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be updated to be obj, where?

@shimks shimks force-pushed the fix-cli-template branch 2 times, most recently from ee786ad to 8455e89 Compare July 13, 2018 15:53
@shimks shimks merged commit c875707 into master Jul 13, 2018
@shimks shimks deleted the fix-cli-template branch July 13, 2018 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI] - lb4 controller generator misplaced arguments on updateAll
6 participants