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

enable simple log output #4821

Closed
wants to merge 1 commit into from
Closed

Conversation

rawgni
Copy link
Contributor

@rawgni rawgni commented Aug 27, 2016

Short Description:

add -sl / --simplelog to pokecli.py

Fixes/Resolves/Closes (please use correct syntax):

- #4800

@mention-bot
Copy link

@rawgni, thanks for your PR! By analyzing the annotation information on this pull request, we identified @douglascamata, @TheSavior and @mjmadsen to be potential reviewers

@Gobberwart
Copy link
Contributor

Gobberwart commented Aug 27, 2016

This should work... ish, but I'm not a fan of this approach as it's putting logger logic into the main bot client and it should be kept separate. Additionally, it requires a command line switch (should be in config).

Suggest adding something appropriate to config, and setting the logging options in the logger class (both logging_handler and colored_logging_handler) initialization, eg.

if self.config.logging_simple:
    logging.basicConfig(level=logging.INFO, format='%(message)s')

@mjmadsen
Copy link
Contributor

We finally merged #3706. I think it does a similar function. I'll leave up until further review.

@Gobberwart
Copy link
Contributor

@mjmadsen Similar, but doesn't appear to strip date/time, just the "extra" info (process name etc.)

Needs another config option to strip d/t?

@mjmadsen
Copy link
Contributor

@Gobberwart good eye. It would be nice if we could add to #3706 on which parts we want to see (each a separate config var).

@Gobberwart
Copy link
Contributor

Gobberwart commented Aug 27, 2016

@mjmadsen Something like that. Maybe something in config like:

logging: {
    color: true/false,
    datetime: true/false,
    processinfo: true/false,
    otherstuff: true/false
}

@mjmadsen
Copy link
Contributor

@Gobberwart Yeah, that'd be perfect.

@Gobberwart
Copy link
Contributor

@mjmadsen I've done some work on this but I'm not entirely satisfied that the way I've read the config options is the best/correct way to do it. Could you please cast an expert eye over #4832

@Gobberwart
Copy link
Contributor

I think PR #4832 can replace this as it provides even more options in a cleaner way.

@rawgni
Copy link
Contributor Author

rawgni commented Aug 28, 2016

close in favour of #4832

@rawgni rawgni closed this Aug 28, 2016
@rawgni rawgni deleted the simple_log_output branch August 28, 2016 05:08
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.

4 participants