Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logrevision #38

Merged
merged 5 commits into from
Feb 11, 2021
Merged

Logrevision #38

merged 5 commits into from
Feb 11, 2021

Conversation

segaura
Copy link

@segaura segaura commented Feb 11, 2021

Closes #37

NOTE: this pull request is about log, so it spreads over most of the python files, I am not able to test everything (e.g. openhub driver, HPSUD mode) so more peer inspection or peer test is needed

This pull request removes the printd() method

    def printd(self, level, msg):        
        if self.logger:
            if level == 'warning':
                self.logger.warning(msg)
            elif level == 'error':
                self.logger.error(msg)
            elif level == 'info':
                self.logger.info(msg)
            elif level == 'exception':
                self.logger.exception(msg)
        else:
            if self.driver != "HPSUD":
                print("%s - %s" % (level, msg))

as it can be seen it has different problems:

  • level is passed as string
  • stardard logging levels, e.g. DEBUG, are missing
  • if using a logger the logging level is respected, if not everything is printed to stdout discouraging developers to use debug, info and warning ...moreover in this only case the format is different
  • "HPSUD mode" is without log and this is obtained accessing an "external object"

To obtain printd() flexibility, a single logger is instructed to use:

  • FileHandler when -g option requires to save log to file
  • StreamHandler for default behavious when log is written to stdout
  • NullHandler when no log has to be written due to "HPSUD mode"

This "one-only-logger" approach enable all the log to use the same global settings for LOG LEVEL.

Having an effetive LOG LEVEL enables to change it via cli option, the --log_level has been introduced for this.

A single unique formatter is defined in advance.

Two info log line were added as proof of concept, they are one for loading of command definitions JSON file and the other for loading of command trasnlations CSV file.

Default level has been left as-is: ERROR.

In addition to log revision this pull request includes:

  • little cosmetic fix, like typos
  • a "help" improvement, now along each "non-writable" command description a " (readonly)" suffix is attached ...and this is because there are long more writable command than there are readonly command

Many things can be further improved, for instance:

  • starting to log before and during options parsing
  • introducing a method to write a message to stdout and log at the same time
  • enabling logformatter to be read from conf file of cli option
  • wrapping all the residual print("some message") to allow more control and avoid to mix them with real output (e.g. JSON)

@Spanni26
Copy link
Owner

Can you create this request new an merge against the testing branch? I think it would be better. Changes can be testet without harming the master branch.

@segaura segaura changed the base branch from master to testing February 11, 2021 14:20
@segaura
Copy link
Author

segaura commented Feb 11, 2021

I did.
Long better, thank you.

@Spanni26 Spanni26 merged commit 4f0352e into Spanni26:testing Feb 11, 2021
@Spanni26
Copy link
Owner

Merged :)

@Spanni26
Copy link
Owner

Spanni26 commented Feb 11, 2021 via email

@segaura segaura deleted the logrevision branch February 11, 2021 15:39
@segaura
Copy link
Author

segaura commented Feb 11, 2021

Cool, how test is managed?
I ask both to contribute to actual test, both for a possible revision, introducing unit tests, mock and so on.

@Spanni26
Copy link
Owner

Spanni26 commented Feb 11, 2021 via email

@martinbischofff
Copy link

And now, with vannis new project, it will be a dead project soon.

:-o Can you provide me a link to that new project you mentioned? I'm using your project and I'm quite happy with it. I was glad to see the new contributions.

@Spanni26
Copy link
Owner

Spanni26 commented Feb 11, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

log improvement
3 participants