Questions, questions!

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
User avatar
Foocaux
Space Squid
Posts: 78
Joined: Sat Jul 26, 2014 7:14 am

Questions, questions!

#1 Post by Foocaux »

I'm slowly trying to figure out how the various bits of FO's codebase connect with each other, and I've got a two questions I've been meaning to ask for a little while:

Why are the Python modules using print instead of logging.info /.debug /.warning?
It's fairly easy to switch the logging output between stderr, with no special formatting, and outputting to the freeoriond.log with timestamps and everything.

Release builds could then include feedback from the python code inside the appropriate logs by default, which seems more conveninent than going through the command line route as of now.
Can we haz logging? I can put together a patch to that effect fairly quickly, if that's acceptable.


And a c++ question: in the past I found myself changing

Code: Select all

if (this_condition)
    DoThis();
to

Code: Select all

if (this_condition)
    DoThis();
    DoThat();
and expecting DoThat() to be called only if this_condition was true(just like it would in python :P)


Having

Code: Select all

if (this_condition) {
    DoThis();
}
while longer to scan than the first version, does make the original code a lot more maintainable, at least IMO.

As a consequence, I've got into the habit of always bracketing all ifs to avoid that particular pitfall.
However, I noticed that in recent commits a few if statements have been de-bracketed, to coin a phrase.

What's the official FO if bracketing policy?
The small print: Any code contribution I make to FreeOrion is made under GPL v2.0, any graphic contribution is under CC-by-SA 3.0

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: Questions, questions!

#2 Post by adrian_broher »

Foocaux wrote:I'm slowly trying to figure out how the various bits of FO's codebase connect with each other, and I've got a two questions I've been meaning to ask for a little while:

Why are the Python modules using print instead of logging.info /.debug /.warning?
Because it hasn't been done yet. No better argument available. Also It shouldn't connect to stderr but to a log4cpp handler/formatter/whatever is responsible for such a feature in log4cpp.
Foocaux wrote:What's the official FO if bracketing policy?
Remove bracket for single statements. Because it was always done this way (which doesn't mean I support the way it is currently done).
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Questions, questions!

#3 Post by Geoff the Medio »

Foocaux wrote:Release builds could then include feedback from the python code inside the appropriate logs by default, which seems more conveninent than going through the command line route as of now.
print is redirected to the FreeOrion logs already... I don't understand the question?
Having

Code: Select all

if (this_condition) {
    DoThis();
}
while longer to scan than the first version, does make the original code a lot more maintainable, at least IMO.
This is C++, not Python... code appropriately. I don't see how this is a maintainability issue.
What's the official FO if bracketing policy?
There isn't one specifically about after if, but I prefer to avoid wasting vertical space with extra lines for {} whenever reasonable to do so, and the existing policy says to try to be consistent with files you are modifying.

User avatar
Foocaux
Space Squid
Posts: 78
Joined: Sat Jul 26, 2014 7:14 am

Re: Questions, questions!

#4 Post by Foocaux »

Geoff the Medio wrote:print is redirected to the FreeOrion logs already... I don't understand the question?
I stand corrected about the way print is redirected, nevertheless, it's considered bad practice in python to redirect print itself, hence the very existence of logging in the python language.

Geoff the Medio wrote: code appropriately. I don't see how this is a maintainability issue.
Geoff, that sounds like the holy grail for all bugs/maintainability issues ever! :D

Being only human, I'm prone to errors, and bracketing single if statements has been an official style code in most of the commercial projects I worked on precisely for the reason I outlined in the original post.

And no, I didn't set the official styles for those projects, there probably where some other fallible humans there too! :mrgreen:
The small print: Any code contribution I make to FreeOrion is made under GPL v2.0, any graphic contribution is under CC-by-SA 3.0

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: Questions, questions!

#5 Post by Geoff the Medio »

Foocaux wrote:...it's considered bad practice in python to redirect print itself, hence the very existence of logging in the python language.
It doesn't actually redirect print specifically... From PythonAI.cpp:

Code: Select all

        std::string logger_script = "import sys\n"
                                    "import freeOrionLogger\n"
                                    "class debugLogger:\n"
                                    "  def write(self, stng):\n"
                                    "    freeOrionLogger.log(stng)\n"
                                    "class errorLogger:\n"
                                    "  def write(self, stng):\n"
                                    "    freeOrionLogger.error(stng)\n"
                                    "sys.stdout = debugLogger()\n"
                                    "sys.stderr = errorLogger()\n"
                                    "print ('Python stdout and stderr redirected')";
Rather, stdout and stderr are redirected to the AI client's log file.

Since the AIs aren't run directly from a console, there's no way to get output from unmodified print statements, so I don't see any use in preserving the default stdout separately from outputting to the log file.

User avatar
Foocaux
Space Squid
Posts: 78
Joined: Sat Jul 26, 2014 7:14 am

Re: Questions, questions!

#6 Post by Foocaux »

So let me recap:

Question 1: because.
Question 2: because!

Fair enough! :D
The small print: Any code contribution I make to FreeOrion is made under GPL v2.0, any graphic contribution is under CC-by-SA 3.0

User avatar
Foocaux
Space Squid
Posts: 78
Joined: Sat Jul 26, 2014 7:14 am

Re: Questions, questions!

#7 Post by Foocaux »

In my megapatch I just posted there's bound to be a few places where I've added braces to singleton ifs, to clarify - for me! - exactly what's going on in the code. Apologies, but as there isn't an official policy about it - and I haven't got too much time on my hands at the moment - I omitted cleaning them up before patching.

Hope those few extra lines don't cause too much hassle.
The small print: Any code contribution I make to FreeOrion is made under GPL v2.0, any graphic contribution is under CC-by-SA 3.0

User avatar
Cjkjvfnby
AI Contributor
Posts: 539
Joined: Tue Jun 24, 2014 9:55 pm

Re: Questions, questions!

#8 Post by Cjkjvfnby »

Foocaux wrote: Why are the Python modules using print instead of logging.info /.debug /.warning?
It's fairly easy to switch the logging output between stderr, with no special formatting, and outputting to the freeoriond.log with timestamps and everything.
I don't like current logging because it add timestamp to each print and cuts long input (AI_*.log).

But I don't want this part of code to be changed. Prints are simple. And I don't feel that we need any special logging advantages.

Python code written in different styles. As I understand it should be consistent with main python style(PEP8) but progress are slow. I stop my working on it for time of release.

PS. I am new one here too.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

Post Reply