snu-quiqcl / qiwis

QuIqcl Widget Integration Software
MIT License
5 stars 2 forks source link

Aijuh/182/log with logging #206

Closed Aijuh closed 11 months ago

Aijuh commented 11 months ago

This closes #182.

I changed all codes using log channel into the codes using logging in poller.py, dbmgr.py, numger.py. Format using 'f' is changed to '%' in logging to prevent pylint error. All logs' level is logging.info. I think in cofig.json, channel name log should be deleted. But I leave it just in case.

Aijuh commented 11 months ago

I updated codes. Deleted backslash, and changed % format. As for error case message, all the number in logging, it has well defined range. So we cannot know error from its value. Otherwise I added logger messages for error cases if there was none.

BECATRUE commented 11 months ago

Oh, after you finish to apply our reviews, please re-request to us by clicking the above reviewers tab! It will make us check quickly.

Aijuh commented 11 months ago

I changed print to logger. But there is some issue.

I changed 'f' format including data type dictionary db to the '%' format as below by importing json. There might be bad thing by importing json or I misused it. logger.info("The message was ignored because " "the database %s has no such key; name or path.", json.dumps(db))

Also in examples/backend.py, I couldn't figure out what is e!r in print(f"apps.backend.write(): {e!r}"), so I just leave it as before. Can I just set it as logger.info("apps.backend.write(): %s", {e!r})?

BECATRUE commented 11 months ago

There might be bad thing by importing json or I misused it. logger.info("The message was ignored because " "the database %s has no such key; name or path.", json.dumps(db))

I think there is no problem because json.dumps() returns just a string!

BECATRUE commented 11 months ago

Can I just set it as logger.info("apps.backend.write(): %s", {e!r})?

I'm not sure, but I think it doesn't cause a problem. I couldn't find the detailed documentation about !r, but it looks another way to use repr(). In other words, e|r is the same as repr(e).

For more details, please see this documentation and this example. I think it is just a convention to print an exception.

Aijuh commented 11 months ago

Thank you for good reference. I changed {e!r} to %r.

kangz12345 commented 11 months ago

For logging the exceptions, you can consider using exception(). This will (probably) print out the exception stack information as well.

Aijuh commented 11 months ago

Fix codes.

Aijuh commented 11 months ago

I agree with that. I will wait @kangz12345's comment.

Aijuh commented 11 months ago

I refer to logging 's log level. I understand error as problem in a particular function, and warning as possible problem. So In the code below I choose warning because two case has same function for now.

if self.dbNames[name]:
    logger.info("Database %s is set as %s.", name, self.dbNames[name])
else:
    logger.warning("Database %s is not selected.", name)

In the code below I use error because it has different function at that time.

for db in content.get("db", ()):
    if any(key not in db for key in ("name", "path")):
        logger.error("The message was ignored because "
                     "the database %s has no such key; name or path.", json.dumps(db))
        continue
kangz12345 commented 11 months ago

I refer to logging 's log level. I understand error as problem in a particular function, and warning as possible problem. So In the code below I choose warning because two case has same function for now.

if self.dbNames[name]:
    logger.info("Database %s is set as %s.", name, self.dbNames[name])
else:
    logger.warning("Database %s is not selected.", name)

Actually in that case, I think info() is okay because it is a valid expected input (you can unselect the database). Of course, after all, databases should be determined so you can label it as a warning. Conclusion: you can decide which to use. I just wanted to tell you that it is not an unexpected situation.

Aijuh commented 11 months ago

Changed warning to info.

Aijuh commented 11 months ago

I will re-read changed code for later. Sorry for repeated error.

BECATRUE commented 11 months ago

I will re-read changed code for later. Sorry for repeated error.

It's okay😃 I will review the updated parts.

Aijuh commented 11 months ago

Code fixed.

kangz12345 commented 11 months ago

Please apply the remaining parts that we discussed in snu-quiqcl/iquip#74.

Please describe the request in more detail.

Aijuh commented 11 months ago

I understand the request. I will update qiwis with respect to iquip.

Aijuh commented 11 months ago

I will first update until currently merged develop branch of iquip. Give me comments if you want further update.

Aijuh commented 11 months ago

Fixed the code.

Aijuh commented 11 months ago

I fixed it. I should have read it.