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

refactor: copilot project #144

Merged
merged 9 commits into from
Jan 7, 2025
Merged

refactor: copilot project #144

merged 9 commits into from
Jan 7, 2025

Conversation

daabr
Copy link
Contributor

@daabr daabr commented Jan 4, 2025

Fixes in general:

  • Use Copilot terminology from Copilot settings page: "cancel seat" instead of "un/subscribe" and "un/register" (except in the metadata description - where brevity is important)

Fixes in README.md:

Fixes in autokitteh.yaml:

  • Removed obsolete project var GITHUB_ORG
  • Renamed project vars and default Slack channel to be more self-describing
  • IDLE_USAGE_THRESHOLD: changed debug values (10m) to stated default (72h)

Replaced helpers.py with users.py (adapted from new Purrr, like helpers.py)

Fixes in users.py compared to helpers.py:

  • "Helpers" is too generic, always use specific names
  • Never return None when a function declares that it returns only str
  • When calling Slack functions, rely only in try & except SlackApiError, checking resp.get("ok", False) is never reached in case of an error (unlike our old Starlark wrapper)
  • _slack_users function: Python rewrite doesn't need recursion (unlike our old Starlark wrapper)

Fixes in msg.json:

  • UX principle: whenever possible, the "yes" option should be "first" (on the left), and the "no" option should be second (on the right)

Fixes in seats.py:

  • Redundant constants and globals: if used only once, embedded them where they're used
  • Use new project var names from the manifest (more descriptive)
  • Lumped GitHub globals together
  • prune_idle_seats function is unnecessary - embedded in find_idle_seats with a new input boolean
  • t timestamp: renamed to be more self-describing (now), but more importantly eset for each seat instead of using a single initial value (the loop should be quick, but may not be)
  • status line too line, split into 2 lines (readability is usually more important than efficiency)
  • Reversed is_idle to is_active: results in flatter code at the bottom of the loop (if is_active: continue instead of if is_idle: ...)
  • seat_dict: renamed, don't specify types in variable names
  • seat_dict: changed type from dict[str, Any] to dict[str, str] - it's our object, no need to mimic GitHub's nested object structure
  • Replaced "engage seat" (which doesn't mean a lot?) with "interact with user" in function name and comments
  • Removed test-only comment and code
  • Subscribe: use event_type instead of data.type
  • Use more modern Python match-case instead of if-elif-else
  • report function: handle gracefully empty SLACK_LOG_CHANNEL to enable making this project var optional

Note: we don't really need autokitteh.start, we could use the same trick as in the Slack sample project (embed important values e.g. GitHub/Slack user IDs in the interactive message and get them when the user clicks a button), but I guess there's educational value in showcasing autokitteh.start.

Fixes in triggers.py:

  • on_schedule function: print each seat on its own instead of the whole list - more readable
  • _get_logins function: renamed arg, don't specify types in variable names

@daabr daabr requested a review from pashafateev January 4, 2025 18:16
@daabr daabr force-pushed the daniel/refactor-copilot branch from 474461d to 144ee36 Compare January 5, 2025 05:43
github_copilot_seats/users.py Outdated Show resolved Hide resolved
github_copilot_seats/README.md Outdated Show resolved Hide resolved
github_copilot_seats/autokitteh.yaml Outdated Show resolved Hide resolved
github_copilot_seats/seats.py Outdated Show resolved Hide resolved
github_copilot_seats/seats.py Show resolved Hide resolved
github_copilot_seats/seats.py Outdated Show resolved Hide resolved
daabr and others added 8 commits January 6, 2025 18:37
- Enable unit testing (except in the Purrr project - those UT require AK
context)
- Add various optional but important linters to Ruff
- Don't pin pytest and ruff versions, no point in updating it from time
to time like in the AK repo
@daabr daabr force-pushed the daniel/refactor-copilot branch from 015dfee to 8a8bf34 Compare January 7, 2025 02:38
@daabr daabr merged commit bdc9ee1 into main Jan 7, 2025
4 checks passed
@daabr daabr deleted the daniel/refactor-copilot branch January 7, 2025 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants