-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add messages for repo error #1
base: master
Are you sure you want to change the base?
Conversation
Would be great to close my fork. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion, thank you! Added two small comments otherwise this seems ready to merge.
git_version/templatetags/git_tags.py
Outdated
repo.__del__() | ||
return chash | ||
except Exception: | ||
print(f"git_version - trouble with repo path {path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(f"git_version - trouble with repo path {path}") | |
logger.debug(f"Failed to find current commit for {path}") |
Can you replace the print
statement with a logger call as described in https://docs.djangoproject.com/en/3.2/topics/logging/#using-logging? That's probably more appropriate for something published as a library.
Just for the sake of completeness, this would also need
import logging
logger = logging.getLogger(__name__)
somewhere at the start of the file
git_version/templatetags/git_tags.py
Outdated
chash = commit.hexsha | ||
repo.__del__() | ||
return chash | ||
except Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except Exception: | |
except Exception as e: |
Is there a specific reason not to print the actual exception message? Was that too verbose?
Yesterday spotted your message, and updated as requested. |
bump |
If a working app is plopped into a container, there is no .git dir, so the Repo() access raises an exception.
This prints a minimal error message, and a clue to the UI why the commit may not show.