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

docs(controller): examples use controller class #1221

Merged
merged 4 commits into from
Jul 25, 2017

Conversation

dead-horse
Copy link
Member

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

@mention-bot
Copy link

@dead-horse, thanks for your PR! By analyzing the history of the files in this pull request, we identified @huacnlee, @cuyl and @lslxdx to be potential reviewers.

@@ -111,29 +111,29 @@ module.exports = app => {
};
```

### Methods in Controller
### Methods in Controller (not recommend, only for compatbility)

Every Controller is a generation function, whose first argument is the instance of the request [Context](./extend.md#context) through which we can access many attributes and methods, encapsulated by the framework, of current request conveniently.
Copy link
Member

Choose a reason for hiding this comment

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

should change this line too first argument

@@ -198,13 +198,13 @@ It can be seen from the above HTTP request examples that there are many places c
Usually the Query String, string following `?` in the URL, is used to send parameters by request of GET type. For example, `category=egg&language=node` in `GET /posts?category=egg&language=node` is parameter that user sends. We can acquire this parsed parameter body through `context.query`:

```js
exports.listPosts = function* (ctx) {
const query = ctx.query;
* listPosts() {
Copy link
Member

Choose a reason for hiding this comment

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

写完整的 class

exports.listPosts = function* (ctx) {
console.log(ctx.queries);
* listPosts() {
console.log(this.ctx.queries);
Copy link
Member

Choose a reason for hiding this comment

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

如果是 class 就将 class 写完整

Copy link
Member

Choose a reason for hiding this comment

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

这样写阅读者会疑惑的

@@ -123,29 +123,29 @@ module.exports = app => {
};
```

### Controller 方法
### Controller 方法(不推荐使用,只是为了兼容)

每一个 Controller 都是一个 generator function,它的第一个参数是请求的上下文 [Context](./extend.md#context) 对象的实例,通过它我们可以拿到框架封装好的各种便捷属性和方法。
Copy link
Member

Choose a reason for hiding this comment

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

这里也要改描述

@codecov
Copy link

codecov bot commented Jul 21, 2017

Codecov Report

Merging #1221 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1221   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files          29       29           
  Lines         705      705           
=======================================
  Hits          703      703           
  Misses          2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b78b4c...b269b4b. Read the comment docs.

@dead-horse
Copy link
Member Author

修改了

throw err;
const Controller = require('egg').Controller;

module.exports = class UploaderController extends Controller {
Copy link
Member

Choose a reason for hiding this comment

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

应该用 app.Controller 这种方式吧? 我们不是不推荐 require('egg').Controller 么?

Copy link
Member Author

Choose a reason for hiding this comment

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

为什么不推荐?

Copy link
Member Author

Choose a reason for hiding this comment

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

我觉得这样比写一个 function return 要好

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

只是内部看到这个有点奇怪,可能需要替换成内部框架的名字,这个另外想办法把,可以在内部文档构建的时候统一做替换,将 require('egg') 给替换掉

Copy link
Member

Choose a reason for hiding this comment

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

文档问题可以解决

@dead-horse
Copy link
Member Author

这个还有什么问题?

@fengmk2 fengmk2 merged commit c3c9fce into master Jul 25, 2017
@fengmk2 fengmk2 deleted the docs-controller-refactor branch July 25, 2017 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants