Logger app using `logging`

[Aijuh]

This pull request is about #181

I set logger in logger.py as root logger, and logger in qiwis.py as name(main), so log from qiwis.py propagate to logger.py. Made loggingHandler to deal with log message and print to loggerFrame window. log message from qiwis.py printed in logger Frame. Moreover I additionally add new Frame for selecting logging level (ex. DEBUG, ERROR .. ). If it is not needed, I will delete that part of code and pull request again. Thank you.

[Aijuh]

Thank you. I will follow your instruction and fix the code.

[Aijuh]

I changed my code and update it. Should I make another pull request or just use this pull request?

[kangz12345]

You can stay in this PR. The reviewers review and you reply to that and apply the reviews, and we repeat this process. At the end, when the reviewers approve this PR, then you can merge it.

[Aijuh]

Thank you for the detailed review. I fixed the code.

[Aijuh]

I understand that we should not change root logger's level to prevent error. So I set logger name as 'parent'(name can be changed depend on notation rule) and upload the code. logger = logging.getLogger("parent") logger.setLevel(logging.INFO) logger.addHandler(self.handler) logger level is temporarily set as logging.INFO to print logger.info in qiwis.

[BECATRUE]

Can you explain what you planned about a logger name and logging level? I think we should decide them first.

In my opinion, I think we can just define a logger in each module such as qiwis.py and each apps as follows.

logger = logging.getLogger(__name__)

It is the recommended method in the docs.

In terms of a logging level, I think each module is responsible for its logger and in logger.py it should just show the passed logs. If we want to set the minimum logging level in the logger GUI, we can use self.handler.setLevel() referred to the docs.

I wonder what @kangz12345 intended and how @Aijuh are going to implement.

[Aijuh]

I am planning to set logger in logger.py as "parent" and set other module's logger using logging.getLogger("parent").getchild(__name__) (might be changed). If I set logger name like this, logger from child propagate to parent. It works well between logger.py and qiwis.py. Of course I should test for other modules later.

For setting logger level, what I understand about @BECATRUE 's suggestion is each module is responsible for its logger and logger level. In this case what I am planning is as follows.

1. Set logger level of parent logger in logger.py as DEBUG. Then, any logger message from other module will be printed.
2. Set logger level of child logger separately in that module. If we set logger level as ERROR, and input logger as logger.info, the message didn't propagate to parent logger in logger.py.

In case of changing handler's level using self.handler.setLevel(), it can block INFO level logs from DEBUG level logger. But it cannot allow ERROR level logger to get DEBUG level logs. In other words, it cannot lower logger's level. So I think that we may change logger's level directly or fix logger's level as DEBUG, and change handler's level.

I will wait exact name convention of logger from @kangz12345 and @BECATRUE .

[BECATRUE]

Thanks for your detailed explanation👍

First, I strongly agree with your idea about setting the logger levels. Also setting all loggers as subloggers of a specific logger, i.e. parent logger is the best solution in terms of management and danger of setting the level of the root looger.

However, I think it is a problem that logger.py, not qiwis.py has the authority to set up the parent logger. Also it is a big restriction that all users using this package should know that all loggers must be subloggers of the parent logger. So I suggests to move the authority to qiwis.py as follows.

# qiwis.py
qiwisLogger = logging.getLogger("parent")
logger = qiwisLogger.getChild(__name__)  # Same as the current logger

# logger.py
import qiwis
...
logger = qiwis.qiwisLogger

# Other apps
import qiwis
...
logger = qiwis.qiwisLogger.getChild(__name__)

The logger.py is just an app, not a part of qiwis package, so we should remove any dependence on qiwis.py's position.

Finally, about the name of the parent logger, I think 'qiwis' is more appropriate because parent looks a liitle ambiguous.

After listening to Jiyong's opinion, let's make a final decision.

[kangz12345]

Thank you for the discussion. I also talked with @Aijuh offline, and we decided to do one of the following:

1. Use the root logger and modify its level.

This might cause some unexpected behavior when any other module uses the root logger as well. However, such a module also takes a risk that the root logger can be used by any other module. Therefore, we just need to document that this modifies the root logger level.

2. Construct a logger hierarchy and make the apps follow the naming convention.

This does not modify the root logger, so it can avoid the problem of the option 1. However, it cannot catch the logs emitted by the loggers which do not follow the naming convention (consider the loggers of any third-party modules which do not care of qiwis). Moreover, the way to set the logger to be a child logger of qiwis logger is quite inconvenient.

In my opinion, unless there is a convenient way to set the logger hierarchy and catch the third-party apps' logs and there is no more problem with modifying the root logger, the option 1 looks better.

[BECATRUE]

Thanks for suggesting options!

I don't think setting the child logger in option 2 is convenient because we can make a helper function in qiwis.py. But the issue looks critical that we cannot track the logs generated by several third-party modules.

Thus I also agree with option 1.

[Aijuh]

I understand there are two options.

1. Using root logger in logger.py
2. Not using root logger.

3. In 1. we should change root logger's level as DEBUG and manage logger.handler to block message. This solution is quite simple but dealing with root logger may cause problem. There is no serious problem in codes, but if other module also using root logger and we change root logger's level, it would cause problem. If we set root logger's level as DEBUG, all logger's default level is set as DEBUG, which is not a severe problem.

In 2. what I am planning is using naming convention of parent and child relationship in qiwis Apps. And at specific app who wants to read imported module's log we can manually read message and deliver to app's logger. logger.py's logger is the highest parent, and qiwis.py will be its child. And all apps logger is child of qiwis.py's logger.

As @kangz12345 told me that it may be okay to use root level logger and @BECATRUE said that it would be much simpler way, I fixed the code in that way. The root logger's level is now DEBUG. Please inform me if I missed something. Thank you.

[BECATRUE]

Good! And please apply the above reviews about code format and Signaller first. Also, our final PR must meet the GitHub actions i.e. Pylint and unittest. To check the failed cause, please click the 'Details' button in the below GitHub actions.

[Aijuh]

Sorry for the redundant code change. Dealing with pylint.error, I misunderstood location of blank space. I changed commitment to follow code format and add comment about signaller.

[kangz12345]

1. You can install pylint and test it with your own machine (you don't have to push and wait for the GitHub action to finish every time).
2. If you are done and want more feedback, you can re-request the review. If you don't have permission to request a review, please let me know.
3. Is it still not clear about the blank space rules, or now you understand it?

[Aijuh]

• You can install pylint and test it with your own machine (you don't have to push and wait for the GitHub action to finish every time).

I didn't know that, I will do in that way.

• If you are done and want more feedback, you can re-request the review. If you don't have permission to request a review, please let me know.

I re-requested to reviewers.

• Is it still not clear about the blank space rules, or now you understand it?

I understand it. I delete unnecessary blank space.

[kangz12345]

Please check the unresolved comments.

[Aijuh]

Add overridden message because loggingHandler in logger.py override logging.Handler.emit

Delete duplicated comments

[Aijuh]

1. Fix error in format
2. In LoggerApp, handler and logger are not class variable.

[Aijuh]

Unittest failed with following error.

_E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/libi/libinput/libinput10_1.20.0-1ubuntu0.2_amd64.deb 404 Not Found [IP: 52.252.75.106 80] E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?___

It seems that fetch link has error. Can I do something to fix it?

[BECATRUE]

Unittest failed with following error.

_E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/libi/libinput/libinput10_1.20.0-1ubuntu0.2_amd64.deb 404 Not Found [IP: 52.252.75.106 80] E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?___

It seems that fetch link has error. Can I do something to fix it?

Oh, we also realized this problem in other repos. Can you add sudo apt-get update into both Pylint and unittest in 'Install dependencies'? The unittest in iquip will help you!

[Aijuh]

Fixed code with reference to comments.

I will properly use 'a' or 'the'.

[Aijuh]

Fixed comments.

Sorry for the repeated grammer error. I will check every comment with the grammer program later.

Fix pylint and unittest.