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

Add messages for repo error #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

topiaruss
Copy link

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.

@topiaruss topiaruss closed this Nov 15, 2021
@topiaruss topiaruss reopened this Nov 15, 2021
@topiaruss
Copy link
Author

Would be great to close my fork.

Copy link
Member

@Xiphoseer Xiphoseer left a 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.

repo.__del__()
return chash
except Exception:
print(f"git_version - trouble with repo path {path}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

chash = commit.hexsha
repo.__del__()
return chash
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except Exception:
except Exception as e:

Is there a specific reason not to print the actual exception message? Was that too verbose?

@topiaruss
Copy link
Author

Yesterday spotted your message, and updated as requested.
Also gave version headroom for django 4.x.x.

@topiaruss
Copy link
Author

bump

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.

2 participants