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

Fixed some bug #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed some bug #4

wants to merge 1 commit into from

Conversation

liyu4
Copy link

@liyu4 liyu4 commented Dec 25, 2016

1: 修复了删除文件空指针错误的问题
2:增加了监控目录提示
3: 改善了启动和再次启动的提示代码
4:修复了文件改变之后但是终端会提示提示两次的问题
5: 移除了部分代码
6:过滤.git文件(噪音文件)
7: 新增readAppDirectories,获取需要监控的目录

Copy link
Owner

@shanzi shanzi left a comment

Choose a reason for hiding this comment

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

首先感谢您的 PR!

Review 过您的修改之后感觉还请您再整理一下代码,而且有些逻辑跟原来的设计偏差有点大,所以恐怕暂时不能merge。

@@ -40,16 +34,36 @@ func watch(path string, abort <-chan struct{}) (<-chan string, error) {
info, err := os.Stat(fp.Name)
if err == nil && info.IsDir() {
// Add newly created sub directories to watch list
log.Printf("Add newly diectory ( %s )\n", fp.Name)
Copy link
Owner

Choose a reason for hiding this comment

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

Add newly created directory?

Copy link
Author

Choose a reason for hiding this comment

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

比如新增加了一个单元文件,这里是想提示出来这个变化。


if os.IsNotExist(err) {
log.Printf("Dictory (%s) have been removed\n", fp)
log.Println("here=======here")
Copy link
Owner

Choose a reason for hiding this comment

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

删除多余的log?

Copy link
Author

Choose a reason for hiding this comment

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

这个不好意思,昨晚太晚了,忘记删这个了。


haveDir := false
for _, fileinfo := range fileInfos {
if fileinfo.IsDir() == true && fileinfo.Name() != "." && fileinfo.Name() != ".git" {
Copy link
Owner

Choose a reason for hiding this comment

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

感觉作为一个简单的 utility 直接 ignore 掉  .git 并不好。

我觉得在使用过程中指定需要侦听的文件类型就应该够用了。对于 .git 这种需求可以做一个选项出来。直接 hard code 不好。

Copy link
Author

Choose a reason for hiding this comment

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

你这样说也可以,可以选择性的过滤一部分文件,比如说隐藏文件

Copy link
Author

Choose a reason for hiding this comment

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

您之前的方案,会把.git下的文件全部都监控起来,我真的不明白监控.git文件有什么特殊的用意。

"log"
"os"
"os/signal"
"path/filepath"
// "wu/command"
Copy link
Owner

Choose a reason for hiding this comment

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

删除这两个注释?

Copy link
Author

Choose a reason for hiding this comment

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

这个是我的失误,没有clone到github下。回头删掉


// Run the command once at initially
r.command.Start(200 * time.Millisecond)
haveBuild := false
r.command.Start(200*time.Millisecond, haveBuild)
Copy link
Owner

Choose a reason for hiding this comment

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

没看懂 haveBuild 是用来干什么的。。。

Copy link
Author

Choose a reason for hiding this comment

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

目前的提示就是done 但是我想合理的提示应该是编译成功一次之后 之后就是重新启动了 你觉得呢

continue
}

if filepath.Ext(fileinfo.Name()) == ".go" {
Copy link
Owner

Choose a reason for hiding this comment

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

hmmm 这里 hardcode 了 go ,如果想要讲 wu 用在其他地方就没办法了吧。这么做有什么理由么?

Copy link
Owner

Choose a reason for hiding this comment

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

wu 按照我的理解新的逻辑只会侦听有 Go 源文件存在的目录,也不会在新建文件和文件夹出现的时候自动加入侦听。这和以前的逻辑偏差太大。恐怕不能接受

Copy link
Author

Choose a reason for hiding this comment

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

是不是做成配置的形式就可以接受了呢,另外监控go文件应该是合理的,如果想要监控其他的类型,比如html这就需要把这部分从配置里读取出来了。

新建文件夹会加入监听的啊。后半部分你讲的我不是太明白

Copy link
Author

Choose a reason for hiding this comment

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

晚上我们再联系,白天忙上班。

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.

2 participants