neo-project / neo-node

MIT License
229 stars 224 forks source link

featured log #848

Closed Jim8y closed 1 year ago

Jim8y commented 2 years ago
image image image image

Original:

image
coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 2201495427


Totals Coverage Status
Change from base Build 2186768199: 0.0%
Covered Lines: 107
Relevant Lines: 683

💛 - Coveralls
erikzhang commented 2 years ago

I don't like this style. It adds a lot of unnecessary newlines. And I don't like the icons either.

Jim8y commented 2 years ago

I don't like this style. It adds a lot of unnecessary newlines. And I don't like the icons either.

Icon can be removed easily. new lines can be avoided. Formatted log makes it easier to address what is happening.

Jim8y commented 2 years ago

@erikzhang Now icons are removed, long hash characters are abbreviated

erikzhang commented 2 years ago

I'd say I like console log on and console log off. But I still prefer the original whole line in the same color, which looks more intuitive.

nicolegys commented 2 years ago

The log message does not write to file now.

1653985972(1)

1653985857(1)

Jim8y commented 2 years ago

@nicolegys can you test again, i could not repeat the problem, everything works well on my machine.

nicolegys commented 2 years ago

@nicolegys can you test again, i could not repeat the problem, everything works well on my machine.

Jim8y commented 2 years ago

@erikzhang @shargon ready to merge.

Jim8y commented 2 years ago

@shargon can you please review this pr?

superboyiii commented 1 year ago

@shargon Could you review again? Tested OK.

Jim8y commented 1 year ago

@shargon Hey shargon, I am not sure what exactly is your concern with this pr, just answer the following two previously unaddressed questions:

Only one Console.Write?

Updated into one line.

Now that it's all the same color, do we still need to parse the message?

Indeed we still need to parse the message, it is a historic issue as the previous log has messages sticked together while we need to separate them apart.

vncoelho commented 1 year ago

I like console log on and console log off. But I still prefer the original whole line in the same color, which looks more intuitive.

@erikzhang, your review is blocking. Can we proceed like this or you still want it to be off as standard?

vncoelho commented 1 year ago

@Liaojinghui, I think there should be some command to remove colors and keep original compatibility. Perhaps some people will prefer it clean as the original one.

Jim8y commented 1 year ago

@Liaojinghui, I think there should be some command to remove colors and keep original compatibility. Perhaps some people will prefer it clean as the original one.

It is already in the black/white mode for the most log, only errors will show in red and warning in yellow, the rest remains black/white

image

vncoelho commented 1 year ago

Let me try again. Last time I saw it differently.

vncoelho commented 1 year ago

Tested again with long lines and works fine. Colors are just for warning and error and it is almost the same as default. Easier for futures control and modifications as well.