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

qt_istat: remove "Error" from logger strings #3194

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

hansu
Copy link
Member

@hansu hansu commented Nov 27, 2024

A warning log message should not include the string "Error". And for critical log messages it is redundant.

A warning log message should not include the string "Error". And for critical log messages it is redundant.
@andypugh andypugh merged commit 05ce127 into LinuxCNC:master Nov 28, 2024
10 checks passed
@c-morley
Copy link
Collaborator

What are you trying to fix?
The word 'error' was the correct use.
for instance:
'warning: Valid Extension Parsing Error' is the opposite of 'warning: Valid Extension Parsing'

@hansu
Copy link
Member Author

hansu commented Nov 29, 2024

'warning: Valid Extension Parsing Error' is the opposite of 'warning: Valid Extension Parsing'

Oh yes in this case you are totally right as well as in some further. That's why I assigned you...

My starting point was this log message:

[Gmoccapy.QTVCP.QT_ISTAT][WARNING]  INI Parsing Error, No MAX_SPINDLE_0_SPEED Entry in DISPLAY, Using: 2500 (qt_istat.py:532)

And for this, the "Error" does not really fit into a warning.

So IMHO either

[Gmoccapy.QTVCP.QT_ISTAT][WARNING]  INI Parsing: No MAX_SPINDLE_0_SPEED Entry in DISPLAY, Using: 2500 (qt_istat.py:532)

or

[Gmoccapy.QTVCP.QT_ISTAT][ERROR]  INI Parsing (Error), No MAX_SPINDLE_0_SPEED Entry in DISPLAY, Using: 2500 (qt_istat.py:532)

But for most of the others, the pattern is different and simply removing the "Error" does not work.
So feel free to revert this commit...

@c-morley
Copy link
Collaborator

c-morley commented Dec 1, 2024

I'm just interested in what the best message for users. It didn't occur to me that a warning with the word error would be confusing or does it just 'feel' wrong?

This is how I intended it (I think):
'Warning (not super important): Syntax error in the parsing of INI file, missing something'

I would say I have been pretty loose in the definition of message levels.
The levels available are: VERBOSE, DEBUG, INFO, WARNING, ERROR, CRITICAL
Maybe we could roughly define them to help with consistency?

@hansu
Copy link
Member Author

hansu commented Dec 2, 2024

This is how I intended it (I think):
'Warning (not super important): Syntax error in the parsing of INI file, missing something'

I agree if it is a warning about a syntax error, but if it's just a message about a missing entry, then I would tend to
log.warning('INI Parsing: No {} Entry in {}, Using: {}'.format(detail, heading, default))

So I would suggest to revert everything of this PR except that line - are you fine with that?

Maybe we could roughly define them to help with consistency?

Yes, we can add it to your document https://github.com/LinuxCNC/linuxcnc/blob/gui-reference-docs/docs/src/gui/gui-dev-reference.adoc

@andypugh
Copy link
Collaborator

Is there a way to revert from the Github interface?

@hansu
Copy link
Member Author

hansu commented Dec 14, 2024

There is a revert button a bit up, but not sure if this is better than a manual revert...
grafik

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.

3 participants