Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

featured log #848

Merged
merged 40 commits into from
Feb 27, 2023
Merged

featured log #848

merged 40 commits into from
Feb 27, 2023

Conversation

Jim8y
Copy link
Contributor

@Jim8y Jim8y commented Jan 22, 2022

image

image

image

image

Original:

image

@coveralls
Copy link

coveralls commented Jan 22, 2022

Pull Request Test Coverage Report for Build 2201495427

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 15.08%

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

💛 - Coveralls

neo-cli/CLI/Logger.cs Outdated Show resolved Hide resolved
neo-cli/CLI/Logger.cs Outdated Show resolved Hide resolved
@Jim8y Jim8y changed the base branch from master to develop February 8, 2022 19:32
@erikzhang erikzhang changed the base branch from develop to master May 19, 2022 02:20
@erikzhang
Copy link
Member

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

@Jim8y
Copy link
Contributor Author

Jim8y commented May 25, 2022

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
Copy link
Contributor Author

Jim8y commented May 25, 2022

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

neo-cli/CLI/Logger.cs Outdated Show resolved Hide resolved
neo-cli/CLI/Logger.cs Outdated Show resolved Hide resolved
neo-cli/CLI/Logger.cs Outdated Show resolved Hide resolved
@Jim8y
Copy link
Contributor Author

Jim8y commented Sep 13, 2022

@shargon can you please review this pr?

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

It looks much better, specially for consensus logs. I am in favor of this feature.

neo-cli/CLI/MainService.Logger.cs Show resolved Hide resolved
@superboyiii
Copy link
Member

@shargon Could you review again? Tested OK.

@Jim8y
Copy link
Contributor Author

Jim8y commented Feb 10, 2023

@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
Copy link
Member

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?

@erikzhang erikzhang self-requested a review February 23, 2023 16:22
@erikzhang erikzhang dismissed their stale review February 23, 2023 16:23

I don't like color, but I don't object to it either. If you like it, please keep going.

@vncoelho
Copy link
Member

@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
Copy link
Contributor Author

Jim8y commented Feb 23, 2023

@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
Copy link
Member

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

@vncoelho vncoelho merged commit 09f1dfd into neo-project:master Feb 27, 2023
@vncoelho
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants