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

Clean logging option #3706

Merged
merged 2 commits into from Aug 27, 2016
Merged

Clean logging option #3706

merged 2 commits into from Aug 27, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 12, 2016

Short Description:

Adds an option to enable clean logging. This will strip all meta data from the logging messages like the name of the process, the name of the event, etc.

Fixes

@ghost ghost mentioned this pull request Aug 12, 2016
@nanogirl21
Copy link

Thank you!

{"location": "Bialik 150, Ramat Gan"},
{"location": "Ayalon Highway, Ramat Gan"},
{"location": "32.091280, 34.795261"}
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not know, but I added the file again

@elicwhite
Copy link
Contributor

@douglascamata Please review this

@douglascamata
Copy link
Member

@extink you only updated the coloured logging handler, please update the normal one too. Also, please, make a nested configuration for logs, something like:

"log": {
    "color": true,
    "clean_mode": true
}

Take a look at the forts configuration code to learn how to do it.

@ghost
Copy link
Author

ghost commented Aug 13, 2016

@douglascamata, done!

@nanogirl21
Copy link

Any idea when this will be merged with the dev? It's pretty low risk as all it's doing is cleaning up the output log.

@douglascamata
Copy link
Member

I think your merge was messed up. This PR now has 33 changes files with TONS of changes. Was this intended?

@ghost
Copy link
Author

ghost commented Aug 13, 2016

Nope, that was not intended.

Cleaned up the commits on this PR. Should be correct now. If not, let me hear!

logging.getLogger("rpc_api").setLevel(log_level)

if self.config.logging_clean and not self.config.debug:
formatter = Formatter(fmt='[%(asctime)s] %(message)s', datefmt='%H:%M:%S')
Copy link
Contributor

@BriceSD BriceSD Aug 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@extink Do you think it could be possible to have the choice of what to show in the settings ? I like seeing the name, but I totally understand that’s superfluous to most user. Having the choice with this setting by default seems a good compromise to me.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BriceSD I think you tagged me by mistake, but I would also like a choice if possible.

This is very clustered for me.
2016-08-13 14:07:53,999 [PokemonCatchWorker] [INFO] [pokemon_appeared] A wild Tentacool appeared! [CP 308] [Potential 0.6] [A/D/S 10/2/15]

This is a million times better
2016-08-13 14:07 [pokemon_appeared] A wild Tentacool appeared! [CP 308] [Potential 0.6] [A/D/S 10/2/15]

Also, not sure if its the code or my terminal but if the date could be changed to 08-13-2016 (MM/DD/YYYY) and the time to 12 hour format I would be really happy. I know that USA is probably the only ones that use this date/time format.

@douglascamata
Copy link
Member

Is this PR dead or alive?

@nanogirl21
Copy link

@extink what is the delay in this? will this EVER be approved for merge?

@ghost
Copy link
Author

ghost commented Aug 16, 2016

@douglascamata: I was waiting for a respone from you guys after my last commit. I had asked if everything was as you guys wished for. If I need to add anything I would like to hear what!

@BriceSD
Copy link
Contributor

BriceSD commented Aug 16, 2016

@extink am I not a guy ? :<

@ghost
Copy link
Author

ghost commented Aug 16, 2016

@BriceSD, did not know you were one of the 'guys'. But if you would like that implemented, I can do that for you ;)

@k4n30
Copy link
Contributor

k4n30 commented Aug 17, 2016

@extink merge conflicts again

@nanogirl21
Copy link

@extink any update??

@k4n30
Copy link
Contributor

k4n30 commented Aug 21, 2016

@nanogirl21 waiting on devs to merge

@douglascamata @TheSavior @BriceSD ping - PR ready for review
Can we ping mention bot?

@elicwhite
Copy link
Contributor

Since this one is logging stuff, I defer to @douglascamata.

@ghost
Copy link
Author

ghost commented Aug 25, 2016

@TheSavior & @douglascamata hope you guys can review this one

@solderzzc
Copy link
Contributor

close since no activity in 13days/

@solderzzc solderzzc closed this Aug 26, 2016
@ghost
Copy link
Author

ghost commented Aug 26, 2016

Ehm, why? I have rebased this pr almost every day for the merge...

@nanogirl21
Copy link

That is batshit insane!

@k4n30
Copy link
Contributor

k4n30 commented Aug 27, 2016

@solderzzc ?
@extink either rebase (again I know), or recreate a new one

@k4n30 k4n30 reopened this Aug 27, 2016
@solderzzc
Copy link
Contributor

@k4n30 need manually fix the conflict. hmm. .

@mjmadsen
Copy link
Contributor

I think it's that the rebases weren't showing up, so it looked like it was dead.

@duttonw
Copy link
Contributor

duttonw commented Aug 27, 2016

If there is still issues, I'll try and help make a clean pull request for this. Hopefully with your name on it all later tonight aest time

@ghost
Copy link
Author

ghost commented Aug 27, 2016

And again, rebased the branch ;)

@mjmadsen
Copy link
Contributor

mjmadsen commented Aug 27, 2016

I'll wait for CI check then merge. Sorry about the delay @extink

@mjmadsen
Copy link
Contributor

mjmadsen commented Aug 27, 2016

CI passed. Merging!

Thanks for the PR, again sorry it took so long to merge.

@mjmadsen mjmadsen merged commit 566a3f7 into PokemonGoF:dev Aug 27, 2016
@ghost
Copy link
Author

ghost commented Aug 28, 2016

@mjmadsen the rebase wil not usually show up, because it only rebases the current commits, without changing the timestamps. But thanks for the merge!

@mjmadsen
Copy link
Contributor

@extink I think that's why he closed it. Looked like it hadn't been touched in quite a few days. Again, sorry for the delay.

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

Successfully merging this pull request may close these issues.

9 participants