Page MenuHomeSoftware Heritage

cli: Some cleanup and add missing options in db-init
ClosedPublic

Authored by anlambert on May 14 2019, 4:16 PM.

Details

Summary

A couple of cleanups for the main cli implementation.

I also added missing help option and log level processing in db-init subcommand

Diff Detail

Repository
rDCORE Foundations and core functionalities
Branch
cli-log-level-decorator
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5710
Build 7806: tox-on-jenkinsJenkins
Build 7805: arc lint + arc unit

Event Timeline

Update:

  • cli: Remove use of private members from the logging module
  • cli.db: Activate help options and set log level
anlambert retitled this revision from cli: Modify log level option management to cli: Some cleanup and add missing options in db-init.May 15 2019, 1:50 PM
anlambert edited the summary of this revision. (Show Details)

Update:

  • cli: Remove use of private members from the logging module

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

  • cli.db: Activate help options and set log level

This one is indeed a good catch.

swh/core/cli/db.py
50–54

This should not be needed there since the logging is set up in the main 'swh' group...

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

Maybe but this is a bad practice and should be avoided.

swh/core/cli/db.py
50–54

What do you mean ? Every sub-commands proceed like this.

swh/core/cli/db.py
50–54

Oh I see. If the log level is set to the root swh namespace, it will be inherited for sub ones ? This means we can remove the call to setLevel in subcommands ?

Update: no need to set the log level again in db-init

swh/core/cli/db.py
50–54

yes. There are subcommands that do logging config stuff, but it is in general more than just a call to setLevel() (eg. in scheduler when calling setup_log_handler and so) but this is "just" some legacy parts that should be improved at some point IMHO.

Update: no need to pass click context in db-init

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

Maybe but this is a bad practice and should be avoided.

<slightly_bad_faith_ mode>like keeping a hand written copy of a piece of data that is in fact defined somewhere else </>

I don't really understand why you did this. What is the problem with the current implementation. Sure it uses a 'private' API, but the logging module won't change any time soon...

Maybe but this is a bad practice and should be avoided.

<slightly_bad_faith_ mode>like keeping a hand written copy of a piece of data that is in fact defined somewhere else </>

TBH both are "bad practice and should be avoided" but I am more reluctant to duplication than using "private" functions, AFAIC.

Let someone else give the thumbs up/down

Real word example of why using private members is a bad idea: https://github.com/Tulip-Dev/tulip/commit/cbb85d65da2cb262e0c20f2b9b16603b8a633aed#diff-675480f07686e76ad2b178ce3b9ccd4e

When Python 3.7 was released, some signatures in his private C API were updated.
We were using them to register internal Python modules written in C to an embedded interpreter (gdb did also the same, I found the proper fix from their mailing list) and thus compilation broke.

Ok this is a C example but the same could happen for Python code.

Considering the only duplicated thing is the name of the levels, this is not a big deal IMHO.

Considering the only duplicated thing is the name of the levels, this is not a big deal IMHO.

Will that prevent the cli interface from breaking (say against the day they remove the internal variable or change the content of that variable)?

@douardda Do you know why the authors make logging._nameToLevel private in the first place?

Will that prevent the cli interface from breaking (say against the day they remove the internal variable or change the content of that variable)?

Considering the log level names are standard and will not change, this will indeed prevent the cli from breaking if the logging module
implementation changes.

swh/core/cli/__init__.py
10–11

I think the WARN level could be removed as it is not listed here https://docs.python.org/3/howto/logging.html#logging-levels

swh/core/cli/__init__.py
10–11

yub yub

This revision is now accepted and ready to land.May 15 2019, 3:20 PM

Update: Remove the WARN entry from the levels list and sort it in ascending order by level priority

This revision was automatically updated to reflect the committed changes.

Real word example of why using private members is a bad idea: https://github.com/Tulip-Dev/tulip/commit/cbb85d65da2cb262e0c20f2b9b16603b8a633aed#diff-675480f07686e76ad2b178ce3b9ccd4e

When Python 3.7 was released, some signatures in his private C API were updated.
We were using them to register internal Python modules written in C to an embedded interpreter (gdb did also the same, I found the proper fix from their mailing list) and thus compilation broke.

Ok this is a C example but the same could happen for Python code.

We can find zillions of such examples for both 'bad practices', so this is no valid argument IMHO.
The risk here was that the usage of this private function breaks as a result of it being modified or killed, which would be immediately detected.

On the other side, having this hand written copy of the list of accepted/possible values that turns being out of sync because of such an evolution in the logging module, as well as not being able to cope with added log levels by 3rd party modules, is much harder to handle once it occurs.

We can find zillions of such examples for both 'bad practices', so this is no valid argument IMHO.

Yes it is, using private API or members can often lead to break builds or scripts execution.
So you will lose time to fix them and it could have been avoided by sticking to the public API
(sometimes you do not have a choice to implement some hacks I know but if there is any alternatives that
prevent the use of private things, it should be preferred)

On the other side, having this hand written copy of the list of accepted/possible values that turns being out of sync because of such an evolution in the logging module

Logging levels did not evolve between Python 2.7 [1] and Python 3.7 [2] so I think we are pretty safe here

as well as not being able to cope with added log levels by 3rd party modules, is much harder to handle once it occurs.

One solution to make everyone happy and handle all possible cases would be to use the following:

LOG_LEVEL_NAMES = [
    logging.getLevelName(v) for v in
    sorted(getattr(logging, '_levelToName', []))
] or ['NOTSET', 'DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL']

[1] https://docs.python.org/2.7/library/logging.html#logging-levels
[2] https://docs.python.org/3/howto/logging.html#logging-levels