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

LogLevelToName are exported,Flags support #3

Closed
wants to merge 9 commits into from
Closed

LogLevelToName are exported,Flags support #3

wants to merge 9 commits into from

Conversation

achun
Copy link

@achun achun commented Aug 5, 2013

LogLevelToName are exported for setting name of log level.
append logger.Flags field for setting log flag; defaults to log.LstdFlags.

LogLevelToName are exported,Flags support
LogLevelToName are exported for setting name of log level.
append logger.Flags field for setting log flag; defaults to log.LstdFlags.
LogLevelToName are exported for setting name of log level.
append logger.Flags field for setting log flag; defaults to log.LstdFlags.
@monnand
Copy link
Member

monnand commented Aug 7, 2013

Thanks for your pull request!

A few questions:

  • Is there any reason why we want to expose logLevelToName to the outside world? I mean, it is a global variable. Exposing it may lead to possible race conditions. (It is supposed to be a read only variable once we imported the package.) It might be a valid argument that users may want to change the level name to their native language, but suppose we want users be able to do it, we may want them to change it by a function call so that there won't be concurrency bugs. (Although I'm not sure if changing "Error" to "错误" would really help Chinese people to understand their logs. It does introduce some encoding issues in some system, as far as I know, making it hard to be parsed and analysed.)
  • Providing a way to change the flag makes more sense. But we need to think an easy way for users. Imagine if we added a Flag field like what we did in this patch, then how would users change their flag? Here is some imaginary code snippet:

import (
    "github.com/uniqush/log"
    stdlog "log" // Yes, we have to change the imported name here.
)

func SomeFunc(logger log.Logger) {
    logger.Flag = stdlog.LstdFlags
}

One big problem I see here is that the user has to know how the flags are defined in the standard log package while they are actually using a totally different log facility. They are supposed to use an eaiser-to-use log package but it ends by forcing users to learn stuffs from two packages. i.e. they have to know what LstdFlags mean and if they want to know more flags, they have to read the standard log package.
Another problem I see is that the user cannot even access the Flag field. Because Flag is defined in structure logger, which is a private structure. The user can only access Logger interface, not a specific structure.

@monnand
Copy link
Member

monnand commented Aug 7, 2013

If you want to make a contribution to this package, I do see a problem with it. Any log operation is not actually concurrent safe. You may want to add a lock to the logger structure.

@achun
Copy link
Author

achun commented Aug 7, 2013

你好 monnand.
我英文太差, 看到你亲切的面容.终于可以拼音说话了.
导出 LogLevelToName 的原因很简单.
就是为了在日志中可以修改 Level 默认对应的词.比如 [Warning]换成[W]或者[Warn].
添加 Flags字段并默认设置成 log.LstdFlags ,目的很明确,比如使用者可以设置

logobject.Flags = stdlog.LstdFlags | stdlog.Lshortfile

当然,这些官方包的flag值,这个log完全可以复制一份,避免 stdlog 这种不优雅的用法.
LogLevelToName 的全局性造成竞争是个问题. 但是对于 log 这个场景来说,好像问题不大啊,
一般不会有人在运行过程中,不断的生成销毁 logger 吧.也不会在运行中不断的更改 LogLevelToName 吧.


正如你所说,抱歉,我只想到开放这个Flags,忘记写操作接口了.
这个接口是给NewLogger多增加个参数好呢, 还是其他, 确实需要考虑下.
感觉开放Flags是有必要的.或者是增加个SetFlags的接口.

并发的问题,应该不是问题,这就是我认为这个log设计的比较妙的地方,因为并发可以在 writer 这个io.Writer 这个实现中做. 也就是说这个设计把并发控制抛给使用者了.粗看好像不严谨,其实呢,在writer中加个锁啥的,很简单.

@achun
Copy link
Author

achun commented Aug 7, 2013

the code of this pull has problems.
I will pull again.

@achun achun closed this Aug 7, 2013
原日志写规则是小于设定的 level 会被写日志, 新增等于level写日志支持
@achun
Copy link
Author

achun commented Aug 13, 2013

@monnand 你好
依照前面所述的想法, 我尝试了一些调整.
https://github.com/achun/log/blob/master/log.go#L153
全部的调整有

  1. LogLevelToName 导出, 以便用户替换级别的字面名称
  2. NewLogger 增加了可变参数 flags, 以便用户设置对应官方包 logger.flag 属性
  3. 增加了日志级别相等才写日志的支持, 这个值设置成-2,用户可以混合在 flags 参数中. 不会有冲突

如果这些调整不会和 uniqush 的初衷冲突的话, 我希望可以合并分支, 当然这需要 uniqush 的意见.
期待你们的回复.

@monnand monnand reopened this Aug 13, 2013
@achun
Copy link
Author

achun commented Aug 17, 2013

抱歉,忘记说了
https://github.com/achun/log/blob/master/log.go#L26
把官方的那些flag常量复制了一份,这样用户就不要import官方log包了

@monnand
Copy link
Member

monnand commented Aug 17, 2013

@achun 不好意思,最近比较忙,一直没来得及回复。鉴于你一次做了三处修改,我不太好全部都加进去。因为有些我觉得还有待讨论,或者需要修改一些接口。

如果方便,能不能把这三处修改分别做成三个pull request,这样我们方便讨论,也可以及早merge一些修改。

@achun
Copy link
Author

achun commented Aug 18, 2013

@monnand
后续的几次pull是我更新自己的分支后,github自动pull的.
分开做没问题,我建立一个分支重新整理下,一次更新一个,讨论完了再下一个.

  1. LogLevelToName 导出, 以便用户替换级别的字面名称
  2. 给 NewLogger 增加了可变参数 flags, 以便用户设置对应官方包 logger.flag 属性,
  3. 增加了日志级别相等才写日志的支持, 这个值设置成-2,用户可以混合在 flags 参数中. 不会有冲突

这3项调整,1是独立的,2,3是有顺序的,3基于2. 我感觉 1 可以先放放, 2优先讨论比较合适.稍候我,pull一个版本

@achun
Copy link
Author

achun commented Aug 22, 2013

抓狂了, 不知道怎么回事, 我那个版本 go get 出来的 HEAD总是指向 fork 最初的位置
如果用 git clone 就没有问题
我知道了,是因为有其他的分支造成的,只留下一个分支就没有问题了

@achun
Copy link
Author

achun commented Aug 23, 2013

新的pull已经提交

@achun achun closed this Aug 23, 2013
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