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

Console styles improvements #17

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

GerkinDev
Copy link

Improve support for styles.

Since styles interpolation is made only on the 1st argument in console.log, this PR had to do a whole refactor of rendering process.
1st iteration converts Serilog tokens to console.log-aware tokens, that are then iterated to build the 1st parameter's string & following arguments.

Some tests included

… along with console args

Adds %o, %s, %c, etc in the template message (1st arg) & actual values as following parameters. More style-friendly
@GerkinDev GerkinDev changed the title Styles improvements Console styles improvements Jan 5, 2022
@GerkinDev
Copy link
Author

Oh, I had the exact same build error than the CI.

@GerkinDev
Copy link
Author

I just found a way to implement the exact same behavior but much closer to your original implementation. Working on it.

@GerkinDev
Copy link
Author

Here you are ! Feedbacks welcome

@nblumhardt
Copy link
Member

Thanks for sending this. CI should be fixed now, so I'll close and reopen to trigger a build.

I'm not sure when I'll have time to review this one - will try to loop back ASAP, sorry!

@nblumhardt nblumhardt closed this Jan 19, 2022
@nblumhardt nblumhardt reopened this Jan 19, 2022
@GerkinDev
Copy link
Author

Hi, I've just spotted a couple of logical errors in my code while re-reviewing my PR. Unfortunately, I can't say if I'd be able to fix it soon since I've moved on non c# projects.

So, I guess I'll do another round on this. I'll take the opportunity to apply the feedbacks you will give me in the meantime.


Personal note here: I'm creating my business and I'm the only tech. It's been some time. If you had the time to make a proper peer review, this would be greatly appreciated. Working mostly alone has some unpleasant side effects. That's why I try to do as many PR as I can.

Have a nice day!

@GerkinDev
Copy link
Author

This should be fixed now. Please review it, and tell me anything I should do to improve this PR.
I think it could be merged.

@GerkinDev
Copy link
Author

Hi @nblumhardt,

While I use the bin built from my fork in my main project, it would be nice to remove it and rely on the nuget package of your lib. There's absolutely no urge, I just want to ping you in case you forgot this PR is here.

Cheers !

@nblumhardt
Copy link
Member

Thanks for the reminder @GerkinDev; I'm unfortunately not able to devote much time to this sink, currently (wondering whether any of the other Serilog maintainers might be spending more time with Blazor and have some thoughts on the direction we might go here? 🤔 )

If all else fails, releasing a fork from your repo (say, Serilog.Sinks.StyledBrowserConsole?) might help get the code to more users who would enjoy it.

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