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

Implement logging and Git Gud Error #308

Closed
wants to merge 1 commit into from

Conversation

sahansk2
Copy link
Contributor

Fixes #279

@@ -13,6 +14,7 @@
from gitgud.skills.user_messages import handle_solution_confirmation
from gitgud.skills.user_messages import show_skill_tree

log=logging.getLogger(__name__)
Copy link
Owner

Choose a reason for hiding this comment

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

There should be spaces for assignments: log = ...

@sahansk2
Copy link
Contributor Author

sahansk2 commented Sep 9, 2020

This is very much not ready for review. I will undraft this PR when it is.

@benthayer
Copy link
Owner

@sahansk2 Does this actually solve #279? There are two specific things we wanted to log, can you add them? This is simple enough to get us started and hopefully to get into the habit of logging errors using the logger.

On another note, do you think we should convert all print statements to use the logger?

@benthayer
Copy link
Owner

Also, can you merge master so you can get the GitHub Actions

@sahansk2
Copy link
Contributor Author

sahansk2 commented Sep 9, 2020

This will solve #279, but it doesn't right now. I wanted to make a draft PR ahead of time so that we could double check on progress as it's developed.

The biggest issues that I have right now are basically:

  • How do we set up a single logger to simultaneously log to two different files?
  • How do we automatically log exceptions without wrapping everything in a try-except block? I'm hesitant of doing this simply because it might obscure some exceptions, like how a KeyError in a level's status method causes Git Gud to raise an unrelated exception ("Current level doesn't exist").

We shouldn't convert everything to logging (see here), but we could definitely convert some print messages to logging (e.g. Current level doesn't exist.)

@benthayer
Copy link
Owner

@benthayer
Copy link
Owner

I think for logging, we can take a simple first approach of simply logging things whenever we think it's necessary, so if there's an exception, include a line with logger.info(information)

@sahansk2
Copy link
Contributor Author

sahansk2 commented Sep 9, 2020

@sahansk2 Does this help? https://stackoverflow.com/questions/11232230/logging-to-two-files-with-different-settings

Unfortunately, not really. It would be a bit of a hassle on our end to always output to two loggers, one error.txt and one YYYY-MM-DD-HH-mm.txt.

I just thought of this, though: what do you think about having a wrapper function, gitgud_logger(*args, **kwargs) that sends args and kwargs to two logger objects simultaneously? That would make logging much easier for us, and we can also use the two-file approach with error.txt and the time-specific file.

@benthayer
Copy link
Owner

benthayer commented Sep 9, 2020

I see what you mean. What if when users had errors, we just logged it all to one file and had them send that file to us?

I think that could be good for a first iteration. Right now, we're not even sure how errors will be coming up

@sahansk2
Copy link
Contributor Author

sahansk2 commented Sep 9, 2020

I see what you mean. What if when users had errors, we just logged it all to one file and had them send that file to us?

That could work, but it might be difficult from a development perspective to sift through it and see the most important errors. Having an error.txt with the most recent issue is the best approach for now. A bug tracking system would be nice to have; we'd probably want it to be opt-in (for automatic), or on a case-by-case basis, along with some method to strip identifying information.

@benthayer
Copy link
Owner

This thread was speculative in terms of what we think will be useful. It's better to wait until we know how we can make something useful, so I'm closing this.

@benthayer benthayer closed this Nov 20, 2020
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.

Develop an error logging framework to hide errors from user unless prompted
2 participants