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

Use ilogger colorized prettyprint and analyzer uri #180

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

smoothdeveloper
Copy link

redo #174/#176 on top of @dawedawe #175 (this PR should only show my commit(s) once #175 is merged I think).

I'm not sure how to check the mode that is not using the command line, but I assume; the integration into MSBuild, so please let me know if adjustments are needed there and how I should invoke it in a custom project.

This does two things:

  • only colorizes the log level and analyzer identifier with the log level colouring (red: errors, yellow: warnings, etc.) but not the message (it is generally more readable than swath of red, orange, so long the heading has the color
  • adds the link to the analyzer URI if it is there (it is always in cyan, but can be a problem depending if console background is light or not, we can consider dark blue in such case)

image

Regarding how I initially did it and now, I see we use "vanilla .NET string formatting" instead of interpolation or fsharp formatting specifyier which enforces there are matching arguments.

I was wondering if this was important to use this less safe way due to relying on the logging infrastructure changes done in #175 or we can reconsider going through F# format specifiers without losing on the logging infrastructure integration?

src/FSharp.Analyzers.Cli/CustomLogging.fs Outdated Show resolved Hide resolved
src/FSharp.Analyzers.Cli/Program.fs Outdated Show resolved Hide resolved
@smoothdeveloper smoothdeveloper force-pushed the use_ilogger_colorized_prettyprint_and_analyzer_uri branch from de69ac4 to b1fce92 Compare December 19, 2023 03:21
@smoothdeveloper
Copy link
Author

after rebasing all seems consistent (including what fantomas formatted, versus comment), let see if this gets green.

@dawedawe
Copy link
Contributor

While I like the change, I don't really want to take the risk for the release of the current week.
So let's investigate what's going wrong here without rush.

@smoothdeveloper
Copy link
Author

No hurries, no worries :)

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