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

fix(relevant-warnings) Read PR instead of commit #2128

Merged
merged 4 commits into from
Mar 10, 2025

Conversation

kddubey
Copy link
Contributor

@kddubey kddubey commented Mar 10, 2025

@kddubey kddubey requested a review from a team as a code owner March 10, 2025 05:14
def _fetch_issues_for_pr_file_cache_key(
organization_id: int, provider: str, external_id: str, pr_file: PrFile, *args
) -> tuple[str]:
return hashkey(organization_id, provider, external_id, pr_file.filename, pr_file.sha)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the commit before this implemented PrFile.__hash__ and PrFile.__eq__ and passed pr_file here instead. but that causes the hash key to be the whole PrFile obj, which can include a large patch. keep the key small by manually passing the filename and sha

patch=file.patch,
status=file.status,
changes=file.changes,
sha=file.sha,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is indeed the blob SHA

Copy link
Member

Choose a reason for hiding this comment

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

unintuitive lol

@kddubey kddubey requested a review from jennmueng March 10, 2025 15:55
@kddubey kddubey merged commit 9cf62a3 into main Mar 10, 2025
22 checks passed
@kddubey kddubey deleted the kddubey/relevant-warnings/read-pr branch March 10, 2025 17:18
Copy link

sentry-io bot commented Mar 10, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎

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.

None yet

2 participants