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

Removes the debug warnings from the console #8

Merged
merged 1 commit into from
Jul 8, 2020

Conversation

evertvorster
Copy link
Contributor

This simple little edit hides the spammy debug error messages in the console

Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

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

I'm mixed...so not sure yet. Will sleep on it

@evertvorster
Copy link
Contributor Author

The messages are debug messages. There really should be a --debug switch that can be thrown to turn messages like this on.

Still, for a production we don't need debug messages scrolling around the console obscuring real errors.

@nabaco
Copy link
Member

nabaco commented Jul 7, 2020

@evertvorster - is possible for you to add the switch, instead of just commenting the logs?
You could add just the logic for the switch, and we'll deal later with integrating it with our build system and/or engine.

@evertvorster
Copy link
Contributor Author

@nabaco, I am not a coder, and would not know where to start in making an ifdef for this.

Since I have just commented out the debug message, it's easy enough for a troubleshooter in the future to re-enable to hunt down bugs.

My apologies for not being able to provide a nicer solution to this at this point in time.

@BenjamenMeyer
Copy link
Member

If they're just debug messages then we should send them to the log system at the debug level and the config will take care of it.

@nabaco
Copy link
Member

nabaco commented Jul 8, 2020

@BenjamenMeyer do you know how to do that in the python files?

@BenjamenMeyer
Copy link
Member

@nabaco not sure how it connects back to C/C++ land, but in Python:

import logging

LOG = logging.getLogger(__name__)

LOG.debug("message")

If we could figure out how that connects with Boost and C++ to dump into the logs there that would be awesome. I'm sure there's a way, just haven't done it myself. Boost Python docs might inform us.

Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

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

Let's make these output messages a switch that can be turned on or off, rather than disabling them completely.

Ideally, we could integrate them with the Boost logging on the C++ side. Not sure just how to do that yet, though.

@nabaco
Copy link
Member

nabaco commented Jul 8, 2020

@stephengtuggy @BenjamenMeyer - so what's our verdict on this PR? @evertvorster clearly stated that he's not up for the job to make the switch?
Does one of you guys wish to pick this up?
If not, I say let's reject this PR, and open a ticket based on it, to be implemented in better times.

@evertvorster
Copy link
Contributor Author

@stephengtuggy OK, which is better... spamming the console with useless error messages making the game look unstable, or a nice calm console? Sure, a debug switch can be added, and we can stick that in as an issue on this Repo to be corrected, and reference this PR as a place to start. Would that not achieve both our goals?

@stephengtuggy
Copy link
Contributor

@evertvorster Hmm, I see your point.

At a minimum, I have filed #8 vegastrike/Vega-Strike-Engine-Source#167 and vegastrike/Assets-Masters#13 . I guess that's good enough for now. I'm okay with merging this PR.

@BenjamenMeyer @nabaco @Loki1950 what do you think?

@stephengtuggy stephengtuggy self-requested a review July 8, 2020 19:18
Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

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

Looks good to me, as a short-term fix.

@stephengtuggy stephengtuggy merged commit 2e8901c into vegastrike:master Jul 8, 2020
@evertvorster evertvorster deleted the python_debug_errors branch July 13, 2020 08:21
@stephengtuggy
Copy link
Contributor

@evertvorster this PR fixed vegastrike/Assets-Masters#10 , right?

@evertvorster
Copy link
Contributor Author

Indeed it did. I closed that issue. :)

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.

4 participants