A couple of cleanups for the main cli implementation.
I also added missing help option and log level processing in db-init subcommand
Differential D1463
cli: Some cleanup and add missing options in db-init anlambert on May 14 2019, 4:16 PM. Authored by
Details 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
Event TimelineComment Actions Build is green Comment Actions Update:
Comment Actions Build is green Comment Actions Build is green Comment Actions 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...
This one is indeed a good catch.
Comment Actions
Maybe but this is a bad practice and should be avoided.
Comment Actions Build is green Comment Actions Build is green Comment Actions <slightly_bad_faith_ mode>like keeping a hand written copy of a piece of data that is in fact defined somewhere else </> Comment Actions TBH both are "bad practice and should be avoided" but I am more reluctant to duplication than using "private" functions, AFAIC. Comment Actions 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. 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. Comment Actions
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? Comment Actions
Considering the log level names are standard and will not change, this will indeed prevent the cli from breaking if the logging module
Comment Actions Update: Remove the WARN entry from the levels list and sort it in ascending order by level priority Comment Actions Build is green Comment Actions We can find zillions of such examples for both 'bad practices', so this is no valid argument IMHO. 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. Comment Actions
Yes it is, using private API or members can often lead to break builds or scripts execution.
Logging levels did not evolve between Python 2.7 [1] and Python 3.7 [2] so I think we are pretty safe here
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 |