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

feat: BaseContextClass add logger #816

Merged
merged 8 commits into from
Apr 28, 2017
Merged

feat: BaseContextClass add logger #816

merged 8 commits into from
Apr 28, 2017

Conversation

dead-horse
Copy link
Member

  • move BaseContextClass to egg
  • add BaseContextLogger
Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

BaseContextClass

Description of change

- move BaseContextClass to egg
- add BaseContextLogger
@mention-bot
Copy link

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

* @param {String} pathName - class path name
* @since 1.0.0
*/
constructor(ctx, pathName) {
Copy link
Member

Choose a reason for hiding this comment

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

pathName 只是在 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.

service 和 controller 都用了,这里还是去掉了,靠 egg-core 注入吧,本来想改成显示声明的,不过对 egg-core 的改动有点大,先算了。

@dead-horse
Copy link
Member Author

@popomore 还是讲 base-context-class 移到 egg 里面维护吧,egg-core 没有 logger 对象挺蛋疼的

@dead-horse
Copy link
Member Author

egg-core 里面的先保留,保证兼容

@popomore
Copy link
Member

egg 继承一个吧,这里面 helper 要用。

@codecov
Copy link

codecov bot commented Apr 28, 2017

Codecov Report

Merging #816 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #816   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          25     27    +2     
  Lines         641    666   +25     
=====================================
+ Hits          641    666   +25
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️
lib/egg.js 100% <100%> (ø) ⬆️
lib/core/base_context_class.js 100% <100%> (ø)
lib/core/base_context_logger.js 100% <100%> (ø)

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 9871e45...b7f56d5. Read the comment docs.

@@ -77,6 +77,7 @@ module.exports = app => {
- `this.app`: 当前应用 [Application](./extend.md#application) 对象的实例,通过它我们可以拿到框架提供的全局对象和方法。
- `this.service`:应用定义的 [Service](./service.md),通过它我们可以访问到抽象出的业务层,等价于 `this.ctx.service` 。
- `this.config`:应用运行时的[配置项](./config.md)。
- `this.logger`:logger 对象,上面有四个方法(DEBUG,INFO,WARN,ERROR),分别代表打印四个不同级别的日志,使用方法和效果与[context logger](../core/logger.md#context-logger)中介绍的一样,但是通过这个 logger 对象记录的日志,在日志前面会加上打印该日志的文件路径,以便快速定位日志打印位置。
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

Choose a reason for hiding this comment

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

[context logger] 前后空格

@dead-horse
Copy link
Member Author

看了一下 egg-core 里面没有依赖 BaseContextClass,测 controller loader 也不需要使用这个基类,任意一个 class 就可以了,还是放到 egg 里面来维护吧。 测试有点诡异,本地正常, CI 上跑有一个 case 一直超时。

@popomore

@popomore
Copy link
Member

今天好几个库的 ci 都有超时

@dead-horse
Copy link
Member Author

终于都过了

@dead-horse
Copy link
Member Author

@popomore 这个合了之后发个新版本吧

@popomore popomore merged commit 0757655 into master Apr 28, 2017
@popomore popomore deleted the base-context-logger branch April 28, 2017 09:01
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.

4 participants