refactor: modify the directory of logDir by popomore · Pull Request #8 · eggjs/scripts

popomore · 2017-10-11T08:06:23Z

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

export this.child
support customize stdout and stderr

- export this.child - support customize stdout and stderr

popomore · 2017-10-11T08:06:49Z

lib/cmd/start.js

+    const envPath = env.PATH || env.Path;
    if (envPath) {
      // for nodeinstall
-      envPath = path.join(baseDir, 'node_modules/.bin') + path.delimiter + envPath;


这个原来这样设置有啥用

不是把 node_modules 里面的 node 加入到环境变量么?

但是没加到 PATH

哈... 写漏了。这块当时还在想怎么写测试

这个测不了，nyc 有 hack

之前想过一种，调用 nodeinstall 下载一个 node 版本，然后 app 里面 print 下，但麻烦就放弃了。

codecov-io · 2017-10-11T09:01:16Z

Codecov Report

Merging #8 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #8      +/-   ##
=========================================
- Coverage   97.72%   97.7%   -0.02%     
=========================================
  Files           6       6              
  Lines         132     131       -1     
=========================================
- Hits          129     128       -1     
  Misses          3       3

Impacted Files	Coverage Δ
lib/cmd/start.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 49d8033...1a7805b. Read the comment docs.

popomore · 2017-10-11T09:02:00Z

test/start.test.js

+        let result = yield httpclient.request('http://127.0.0.1:7001/env');
        assert(result.data.toString() === 'pre, true');
+        result = yield httpclient.request('http://127.0.0.1:7001/path');
+        assert(result.data.toString().match(new RegExp(`^${fixturePath}/node_modules/.bin:`)));


只能这样假装测试一下

哈哈哈哈哈

: 这个改为 path.delimiter 吧，万一后面支持 win

popomore · 2017-10-11T09:57:59Z

这个用例 windows 是不是跑不起来

popomore · 2017-10-11T09:59:23Z

lib/cmd/start.js

-function* getRotatelog(logDir) {
-  const stdoutPath = path.join(logDir, 'master-stdout.log');
-  const stderrPath = path.join(logDir, 'master-stderr.log');
+  getFrameworkPath(params) {


暴露了这个方法

要不要改为 generator 的?

哪里是 generator 的？

* getFrameworkPath，这种一般都要去检查文件啥的吧， utils 里面是用 sync 么？

popomore · 2017-10-12T01:26:41Z

@atian25 过了

atian25

+1

atian25 · 2017-10-12T01:39:17Z

lib/cmd/start.js

+
+    const env = context.env;
+    env.HOME = HOME;
+    env.NODE_ENV = 'production';


~~PWD 不传了？~~

看到下面 options 传了

atian25 · 2017-10-12T01:42:41Z

lib/cmd/start.js

-function* getRotatelog(logDir) {
-  const stdoutPath = path.join(logDir, 'master-stdout.log');
-  const stderrPath = path.join(logDir, 'master-stderr.log');
+  getFrameworkPath(params) {


要不要改为 generator 的?

atian25 · 2017-10-12T01:44:27Z

lib/cmd/start.js

-  /* istanbul ignore else */
-  if (yield fs.exists(stderrPath)) {
-    yield fs.rename(stderrPath, stderrPath + timestamp);
+    yield fs.rename(logfile, logfile + timestamp);


这里其实如果能重命名为上次启动的时间，而不是本次的时间会更好一点，避免误导。
但好像 fs 拿文件的 create time 不准？

标记为重启时间也还行

只是感觉会误导的，一般不都是文件名 + 启动时间 么？当时想改的，但没找到获取时间的地方。

不过这里面应该也没什么太多有价值的 log。

atian25 · 2017-10-12T01:45:01Z

package.json

    "cov": "egg-bin cov",
    "lint": "eslint .",
-    "ci": "egg-bin pkgfiles --check && npm run lint && npm run cov",
+    "ci": "egg-bin pkgfiles -- --check && npm run lint && npm run cov",


这里不用 -- 不是 npm scripts。

atian25 · 2017-10-12T01:46:50Z

test/start.test.js

  const fixturePath = path.join(__dirname, 'fixtures/example');
  const homePath = homedir();
-  const logDir = path.join(homePath, 'logs/example');
+  const logDir = path.join(homePath, 'logs');


这样会误删其他 logs 吧？

本来这个目录都要删除吧

这是 home 目录下的吧，如果同时开多个项目，那边还在跑的时候，这个单测就把人家的日志删了。

换成 mock 的目录

嗯，这个目录要不要加到 gitignore 去?

用例会删除的

atian25 · 2017-10-12T01:48:22Z

test/start.test.js

+        let result = yield httpclient.request('http://127.0.0.1:7001/env');
        assert(result.data.toString() === 'pre, true');
+        result = yield httpclient.request('http://127.0.0.1:7001/path');
+        assert(result.data.toString().match(new RegExp(`^${fixturePath}/node_modules/.bin:`)));


: 这个改为 path.delimiter 吧，万一后面支持 win

popomore · 2017-10-12T03:14:14Z

node-modules/node-homedir#4

atian25 · 2017-10-12T03:17:41Z

+1

popomore · 2017-10-12T03:30:58Z

过了

atian25 · 2017-10-12T03:34:00Z

不用 homedir 的新功能?

那你合了发版本吧

popomore · 2017-10-12T03:35:18Z

homedir 已经发布了

atian25 · 2017-10-12T03:46:53Z

顺便 link 下自定义日志的那个 Issue

refactor: modify the directory of logDir

4de9fad

- export this.child - support customize stdout and stderr

popomore commented Oct 11, 2017

View reviewed changes

popomore changed the title ~~refactor: modify the directory of logDir~~ WIP: refactor: modify the directory of logDir Oct 11, 2017

f

597c440

popomore commented Oct 11, 2017

View reviewed changes

popomore changed the title ~~WIP: refactor: modify the directory of logDir~~ refactor: modify the directory of logDir Oct 11, 2017

f

346fd11

popomore commented Oct 11, 2017

View reviewed changes

f

bab863c

atian25 approved these changes Oct 12, 2017

View reviewed changes

popomore added 2 commits October 12, 2017 09:56

f

275421b

f

1a7805b

popomore merged commit cfd0d2f into master Oct 12, 2017

popomore deleted the log branch October 12, 2017 03:35

popomore mentioned this pull request Oct 12, 2017

egg-scripts 的日志目录支持自定义配置吗？ eggjs/egg#1395

Closed

Conversation

popomore commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atian25 Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popomore commented Oct 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popomore commented Oct 12, 2017

Uh oh!

atian25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popomore commented Oct 12, 2017

Uh oh!

atian25 commented Oct 12, 2017

Uh oh!

popomore commented Oct 12, 2017

Uh oh!

atian25 commented Oct 12, 2017

Uh oh!

popomore commented Oct 11, 2017 •

edited

Loading

atian25 Oct 11, 2017 •

edited

Loading

codecov-io commented Oct 11, 2017 •

edited

Loading