chore: Add heuristics for issue tracker by ohmayr · Pull Request #12703 · googleapis/google-cloud-python

ohmayr · 2024-05-16T16:52:42Z

Fixes #11147 🦕
Fixes #12679

vchudnov-g

Looks great! We're almost there. Just a few comments. Most are minor, but I do think it matters we be robust about handling multiple components in the saved search.

Oh, and update the PR description....

vchudnov-g

Two minor code suggestions and two doc suggestions.

Also, could you modify /.github/workflows/main.yml so that it has a workflow_dispatch trigger, so we can kick off this workflow from the GitHub UI when we need to? (It will also be a quick way of seeing this take effect after we merge!) It's just adding a line identical to this one

vchudnov-g · 2024-05-17T16:16:54Z

+# BASE_ISSUE_TRACKER defines the base url for issue tracker.
+BASE_ISSUE_TRACKER = "https://issuetracker.google.com"

+# GENERIC_ISSUE_TRACKER_COMPONENT defines a generic component for issue tracker.


This comment will leave us/others puzzled in the future. Let's briefly expand why we care:

"This issue-tracker component is part of some saved searches for listing API-side issues. However, when we construct URLs for filing new issues (which in some cases we do by analyzing the query string for a saved search), we want to ensure we DON'T file a new issue against this generic component but against a more specific one."

vchudnov-g · 2024-05-17T16:22:21Z

+        for component_id in query_match:
+            if component_id != GENERIC_ISSUE_TRACKER_COMPONENT:
+                return component_id
+        return None


To be safe in case there's more than one non-generic component, what do you think of this?

Suggested change

for component_id in query_match:

if component_id != GENERIC_ISSUE_TRACKER_COMPONENT:

return component_id

return None

target_component = None

for component_id in query_match:

if component_id == GENERIC_ISSUE_TRACKER_COMPONENT:

continue

if target_component:

# We extracted two possible target components. We don't want to use the wrong one.

return None

target_component=component_id

return target_component

vchudnov-g · 2024-05-17T16:29:11Z

-        f"     - |PyPI-{client.distribution_name}|\n",   
+        f"     - {client.release_level}\n",
+        f"     - |PyPI-{client.distribution_name}|\n",
+        f"     - `API Issues <{client.show_api_issues}>`_\n" if client.issue_tracker_component_id else "     -\n",


It would be cleaner to reference that same object in the string and the conditional. Otherwise you're breaking encapsulation, knowing that if there's a component then the resulting string exists.

f" - `API Issues <{client.FOO}>`_\n" if client.FOO else " -\n",

And it might make sense to assign these values to local variables first so you don't call and run any getters repeatedly in these lines. (I don't know whether the interpreter can optimize that away; I would assume not.)

(Sorry, I meant to send this comment last night but I forgot to do the final click)

vchudnov-g · 2024-05-17T23:46:47Z

+    _show_api_issues = client.show_api_issues
+    _file_api_issue = client.file_api_issue


(No action needed--just a comment)
You could also memoize the two properties on the RHS. The complication is that for this application, you need two pieces of information for the memoization (as you implemented above), and that's because the cached value could be None. In many applications, the memoized value is not None, so the value None can stand for "not yet cached". So I don't think it's worth pursuing here, because it adds two fields to the class, and this is certainly clear enough.

vchudnov-g

Just some cosmetic comments. Two of them are non-nit clarifications; please address. The others are nits; up to you.

Thank you so much for doing this! This looks like really solid code.

(Oh, and if you could remove the trailing whitespace from your lines [mostly from the empty lines], that would be a nice-to-have if it's easy to do. Otherwise don't worry about it.)

parthea · 2024-05-22T11:12:22Z

+     - |PyPI-google-cloud-access-approval|
+     -
+     -
+     - `Client Library Issues <https://github.com/googleapis/google-cloud-python/issues>`_


Non-blocking comment. Please feel free to file a bug as follow up work

For projects where we are creating issues directly in google-cloud-python we could update this link to point to template for filing an issue: For example, https://github.com/googleapis/google-cloud-python/issues/new?assignees=&labels=&projects=&template=bug_report.md

One benefit of this is that we can set the labels parameter in the url which is empty by default.

For example, we could add the type: bug label and api: access approval label , using url encoding for the colon and space characters.
https://github.com/googleapis/google-cloud-python/issues/new?assignees=&labels=type%3A%20bug%2Capi%3A%20accessapproval&projects=&template=bug_report.md

As long as the priority remains unset, the bug will be considered un-triaged and will still appear in the un-triaged list.

I suggest we do this as a follow-up after we have the new issue templates set up.

ohmayr added 3 commits May 16, 2024 16:47

determine urls

1105931

fix readme formatting

83e9445

refactor code

e3f47ad

ohmayr changed the title ~~Add heuristics for issue tracker~~ Chore: Add heuristics for issue tracker May 16, 2024

vchudnov-g reviewed May 16, 2024

View reviewed changes

Comment thread README.rst Outdated

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py

parthea assigned ohmayr May 16, 2024

ohmayr added 2 commits May 16, 2024 22:06

add links derived from saved searches

ebc4a64

add template id to file an issue URL

94cd442

ohmayr marked this pull request as ready for review May 16, 2024 22:19

ohmayr requested a review from a team May 16, 2024 22:19

vchudnov-g reviewed May 17, 2024

View reviewed changes

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py Outdated

ignore generic component

3952eb4

vchudnov-g reviewed May 17, 2024

View reviewed changes

parthea assigned parthea and unassigned ohmayr May 17, 2024

parthea reviewed May 17, 2024

View reviewed changes

Comment thread README.rst Outdated

parthea assigned ohmayr and unassigned parthea May 17, 2024

parthea reviewed May 17, 2024

View reviewed changes

Comment thread scripts/updateapilist.py Outdated

parthea changed the title ~~Chore: Add heuristics for issue tracker~~ chore: Add heuristics for issue tracker May 17, 2024

ohmayr added 5 commits May 17, 2024 20:18

address PR comments

65ce226

address PR comments

970cd9e

add error logs to script

a26b680

minor improvements

2e724bd

allow manual triggering of workflow

10a1718

vchudnov-g reviewed May 17, 2024

View reviewed changes

address PR comments

3dc545a

ohmayr assigned parthea and vchudnov-g May 21, 2024

cache properties

cab780e

minor refactor

7da8495

vchudnov-g approved these changes May 21, 2024

View reviewed changes

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py

Comment thread scripts/updateapilist.py Outdated

Comment thread scripts/updateapilist.py Outdated

This was referenced May 21, 2024

backfill .repo-metadata.json files missing support URLs. #12722

Closed

docs: update the list of available APIs #12715

Closed

parthea approved these changes May 22, 2024

View reviewed changes

ohmayr added 3 commits May 22, 2024 17:48

address PR comments

0ab99f2

revert README

782329f

Merge branch 'main' into add-heuristics-for-issue-tracker

e2eaae9

ohmayr merged commit 3650261 into main May 22, 2024

ohmayr deleted the add-heuristics-for-issue-tracker branch May 22, 2024 18:09

vchudnov-g mentioned this pull request May 22, 2024

add issue tracker links to individual packages' READMEs #12729

Closed

-        for component_id in query_match:
-            if component_id != GENERIC_ISSUE_TRACKER_COMPONENT:
-                return component_id
-        return None
+        target_component = None
+        for component_id in query_match:
+            if component_id == GENERIC_ISSUE_TRACKER_COMPONENT:
+                continue
+            if target_component:
+              # We extracted two possible target components. We don't want to use the wrong one.
+              return None
+            target_component=component_id
+        return target_component

		_show_api_issues = client.show_api_issues
		_file_api_issue = client.file_api_issue

Conversation

ohmayr commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vchudnov-g left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ohmayr commented May 16, 2024 •

edited

Loading

vchudnov-g left a comment •

edited

Loading

vchudnov-g May 17, 2024 •

edited

Loading

vchudnov-g left a comment •

edited

Loading