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: modify and fix 3 points #264

Merged
merged 2 commits into from
Jan 16, 2017
Merged

docs: modify and fix 3 points #264

merged 2 commits into from
Jan 16, 2017

Conversation

missjing
Copy link
Contributor

@missjing missjing commented Jan 16, 2017

1)默认脚手架生成的工程「egg-init egg-init egg-example --type=simple」中路由写到的/home了
2)this.ctx.curl(url, { dataType: 'json' })的返回值非promise对象,使用.then()会报错
3)需config中增加serverUrl、pageSize的值

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

1)默认脚手架生成的工程「egg-init egg-init egg-example --type=simple」中路由写到的/home了
2)this.ctx.curl(url, { dataType: 'json' })的返回值非promise对象,使用.then()会报错
3)需config中增加serverUrl、pageSize的值
@mention-bot
Copy link

@missjing, thanks for your PR! By analyzing the history of the files in this pull request, we identified @atian25 and @popomore to be potential reviewers.

```js
// config/config.default.js
config.news = {
pageSize: 5,
Copy link
Member

Choose a reason for hiding this comment

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

这里缩进错了

@@ -217,6 +217,15 @@ exports.list = function* newsList() {
yield this.render('news/list.tpl', { list: newsList });
};
```
还需增加app/service/news.js中读取到的配置:
Copy link
Member

@popomore popomore Jan 16, 2017

Choose a reason for hiding this comment

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

文件路径加上代码块,还有空格

@popomore popomore requested a review from atian25 January 16, 2017 08:33
@codecov-io
Copy link

codecov-io commented Jan 16, 2017

Current coverage is 97.89% (diff: 100%)

Merging #264 into master will not change coverage

@@             master       #264   diff @@
==========================================
  Files            34         34          
  Lines           904        904          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            885        885          
  Misses           19         19          
  Partials          0          0          

Powered by Codecov. Last update f66134e...60ace54

Copy link
Member

@atian25 atian25 left a comment

Choose a reason for hiding this comment

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

就上面 2 点改改, 其他 +1

@atian25
Copy link
Member

atian25 commented Jan 16, 2017

@missjing 麻烦改改重新提交下

@popomore popomore changed the title docs:modify and fix 3 points docs: modify and fix 3 points Jan 16, 2017
@@ -196,9 +196,9 @@ module.exports = app => {
// parallel GET detail, see `yield {}` from co
const newsList = yield Object.keys(idList).map(key => {
const url = `${serverUrl}/item/${idList[key]}.json`;
return this.ctx.curl(url, { dataType: 'json' }).then(res => res.data);
Copy link
Member

Choose a reason for hiding this comment

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

@atian25 叫你不写单测。。。

Copy link
Member

Choose a reason for hiding this comment

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

-.-!!

return this.app.urllib.request(url, { dataType: 'json' }).then(res => res.data);

内网版之前这么写的, 我就随手简化了下... examples 那边没问题的.

@@ -23,7 +23,7 @@ $ npm i

```bash
$ npm run dev
$ open localhost:7001
$ open localhost:7001/home
Copy link
Member

Choose a reason for hiding this comment

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

这个地方不用改 home 了, boilerplate 那边我发 PR 修改了.
https://github.com/eggjs/egg-boilerplate-simple/pull/19/files

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

@atian25 atian25 left a comment

Choose a reason for hiding this comment

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

其他 +1

@atian25
Copy link
Member

atian25 commented Jan 16, 2017

先合并了, 我在另一个 PR 改这个 home

@atian25 atian25 merged commit 0dcfd25 into eggjs:master Jan 16, 2017
@atian25 atian25 mentioned this pull request Jan 16, 2017
4 tasks
@popomore
Copy link
Member

merge 的时候记得也要调整下 message

0dcfd25

@fengmk2
Copy link
Member

fengmk2 commented Jan 16, 2017

呃。。。这个需要 revert 掉。。 @atian25 commit log 不对。。

@atian25
Copy link
Member

atian25 commented Jan 16, 2017

image

没问题吧? 不是 squash 了?

@fengmk2
Copy link
Member

fengmk2 commented Jan 16, 2017

@atian25 你看看。。。

image

@atian25
Copy link
Member

atian25 commented Jan 16, 2017

嗯, 下次注意下.

那还是 revert 吧, 几个修改点的.

@fengmk2
Copy link
Member

fengmk2 commented Jan 16, 2017

revert 不了了,很多人都 pull 了。只能硬接受了。以后谁搞错了罚款500

mattma added a commit that referenced this pull request Jan 17, 2017
* master: (37 commits)
  docs: fix quickstart typo (#266)
  docs: add http client debug docs (#265)
  docs: modify and fix 3 points (#264)
  docs(intro): improve decription (#263)
  docs: fix docs site version (#262)
  docs: Fix typo.  (#261)
  docs: review 1st version docs (#257)
  fix: typo conext -> context (#259)
  docs: contributing && readme && deps (#253)
  docs: fix quickstart link in index.html (#256)
  docs: set the default locale zh-cn (#255)
  refactor: ctx.realStatus delegate ctx.response.realStatus (#252)
  docs: Add intro/index.md (#246)
  feat: adjust default plugins (#251)
  docs: add RESTful documents (#247)
  feat: delegate ctx.jsonp to ctx.response.jsonp (#248)
  chore: remove examples (#245)
  docs: improve mysql doc
  docs: add mysql doc
  docs: view (#228)
  ...
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.

6 participants