-
Notifications
You must be signed in to change notification settings - Fork 322
[XDebug Bridge] Add --quiet
and --verbose
options in bridge
#2436
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
base: trunk
Are you sure you want to change the base?
Conversation
--quiet
and --verbose
options in Xdebug Bridge--quiet
and --verbose
options in bridge
I will try to add tests before marking it as ready for review. |
Maybe |
@adamziel This one was a quite big refactoring. I will need a review on this. A little summary of what I had in mind when I coded these :
I therefore have several questions :
|
const isConsole = handler.name === 'logToConsole'; | ||
const isSeverityAllowed = log.severity | ||
? this.filters.includes(log.severity) | ||
: !!this.filters.length; | ||
|
||
if (!isConsole || isSeverityAllowed) { | ||
handler(log, ...args); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused – what's the reasoning here? Do we need special casing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're asking for the isConsole
line : in my last comment I wanted to know if your suggestion with --verbosity
only concerned logToConsole
. If not I can remove that line.
If you're asking for the isSeverityAllowed
line, the severity of a log can be a LogSeverity type but also undefined
. And if it is undefined, it takes the Info
severity in log-to-memory
. That is why I had to use a ternary operator.
I would suggest to replace the undefined severity by a Log
LogServerity
and in log-to-memory.ts
replace:
- log.severity ?? LogSeverity.Info,
+ log.severity == LogSeverity.Log ? LogSeverity.Info : log.severity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my last comment I wanted to know if your suggestion with --verbosity only concerned logToConsole.
Ah, good question! Let's target all handlers. If we want to restrict the console output while still collecting all the logs somewhere, we can do that at the logToConsole
handler level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Things are looking good! I haven't tested it yet at this stage, only left a few comments – this one seems the most important. |
I'd say normal includes all the log levels except
Potentially? But not in this PR, it already covers a lot of ground. |
Done.
Ok! What do you think about using |
Sounds great for a follow-up PR! |
@@ -96,7 +96,7 @@ export class BlueprintsV1Handler { | |||
); | |||
progressReached100 = percentProgress === 100; | |||
|
|||
if (!this.args.quiet) { | |||
if (this.args.verbosity !== LogVerbosity.Quiet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check it in this.writeProgressUpdate
instead of before every call
I'll test this and follow up tomorrow |
export const LogVerbosity = { | ||
Normal: 'normal', | ||
Quiet: 'quiet', | ||
Debug: 'debug', | ||
} as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not distinguish between verbosity and severity at the logger level. One concept is enough. We can decide what each verbosity means in the CLI args parser when we let the user decide. All internal methods could use severities for filtering.
export const LogSeverity = { | ||
Log: { name: 'log', level: 1 }, | ||
Info: { name: 'info', level: 1 }, | ||
Warn: { name: 'warn', level: 1 }, | ||
Error: { name: 'error', level: 1 }, | ||
Fatal: { name: 'fatal', level: 1 }, | ||
Debug: { name: 'debug', level: 2 }, | ||
} as const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give each severity a unique level, e.g.
export const LogSeverity = {
Fatal: { name: 'fatal', level: 0 },
Error: { name: 'error', level: 1 },
Warn: { name: 'warn', level: 2 },
Log: { name: 'log', level: 3 },
Info: { name: 'info', level: 4 },
Debug: { name: 'debug', level: 5 },
} as const;
I've left two notes about the code design, and my only other note would be to adjust the PR title and description to reflect the larger logging refactor. |
Motivation for the change, related issues
Before this pull request, every logs were printed without control on it.
Now, By default, only the
start-bridge
logs are displayed in the terminal:Implementation details
The
--quiet
option empties thelogger.handlers
and no output is printed.The
--verbose
option will print every log. And mainly theCDP
andDBGP
receive and send logs useful for bridge development but too noisy for actual PHP debugging.Testing Instructions (or ideally a Blueprint)
Normal mode
Quiet mode
Verbose mode