Refactor: beginner, intermediate, advanced labels#2170
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 33 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIssue templates, workflows, and automation scripts were updated to use namespaced skill labels ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/bot-beginner-assign-on-comment.js (1)
258-266: 🛠️ Refactor suggestion | 🟠 MajorBuild the issue-search URL from
BEGINNER_LABELtoo.Line 265 uses the shared constant in the visible text but still hardcodes
skill: beginnerin the query URL, so env overrides or future label renames will send users to the wrong search.♻️ Proposed fix
if (issue.assignees && issue.assignees.length > 0) { try{ const currentAssignee = issue.assignees[0]?.login ?? "another contributor"; + const beginnerIssuesUrl = `https://github.com/${repo.owner.login}/${repo.name}/issues?q=${encodeURIComponent( + `is:issue is:open label:"${BEGINNER_LABEL}" no:assignee` + )}`; await github.rest.issues.createComment({ owner: repo.owner.login, repo: repo.name, issue_number: issue.number, - body: `👋 Hi @${commenter}, thanks for your interest! This issue is already assigned to @${currentAssignee}, but we'd love your help on another one. You can find more "${BEGINNER_LABEL}" issues [here](https://github.com/${repo.owner.login}/${repo.name}/issues?q=is%3Aissue+is%3Aopen+label%3A%22skill%3A+beginner%22+no%3Aassignee).`, + body: `👋 Hi @${commenter}, thanks for your interest! This issue is already assigned to @${currentAssignee}, but we'd love your help on another one. You can find more "${BEGINNER_LABEL}" issues [here](${beginnerIssuesUrl}).`, });As per coding guidelines, “Use top-level constants for configuration — avoid hardcoded values scattered through the script.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa403e7a-5373-4ccb-8e0c-d6e7efc12f27
📒 Files selected for processing (14)
.github/ISSUE_TEMPLATE/02-beginner-issue.yml.github/ISSUE_TEMPLATE/03-intermediate-issue.yml.github/ISSUE_TEMPLATE/04-advanced-issue.yml.github/scripts/bot-advanced-check.sh.github/scripts/bot-beginner-assign-on-comment.js.github/scripts/bot-intermediate-assignment.js.github/scripts/bot-next-issue-recommendation.js.github/scripts/coderabbit_plan_trigger.js.github/scripts/migrate-skill-labels.sh.github/scripts/shared/labels.js.github/scripts/shared/labels.test.js.github/workflows/bot-advanced-check.yml.github/workflows/bot-coderabbit-plan-trigger.yml.github/workflows/bot-intermediate-assignment.yml
6308eda to
3ef0b1e
Compare
DeathGun44
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe749e33-e3a0-479c-8601-7643b9e99c84
📒 Files selected for processing (14)
.github/ISSUE_TEMPLATE/02-beginner-issue.yml.github/ISSUE_TEMPLATE/03-intermediate-issue.yml.github/ISSUE_TEMPLATE/04-advanced-issue.yml.github/scripts/bot-advanced-check.sh.github/scripts/bot-beginner-assign-on-comment.js.github/scripts/bot-intermediate-assignment.js.github/scripts/bot-next-issue-recommendation.js.github/scripts/coderabbit_plan_trigger.js.github/scripts/migrate-skill-labels.sh.github/scripts/shared/labels.js.github/scripts/shared/labels.test.js.github/workflows/bot-advanced-check.yml.github/workflows/bot-coderabbit-plan-trigger.yml.github/workflows/bot-intermediate-assignment.yml
exploreriii
added
reviewer: junior committer
exploreriii
removed
step: 1st
|
@DeathGun44 happy to review again once changes are addressed, thank you! |
1f5a091 to
d61d4a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/bot-next-issue-recommendation.js (1)
169-177:⚠️ Potential issue | 🟡 MinorUse
GOOD_FIRST_ISSUE_LABELconstant for the fallback search query instead of hardcoded label strings.Line 175 hardcodes
'(label:"good first issue" OR label:"skill: good first issue")', which bypasses the centralized label configuration imported at the top of the file. TheGOOD_FIRST_ISSUE_LABELconstant (default:"Good First Issue") should be used here to respect environment variable overrides and maintain consistency. The"skill: good first issue"variant is not defined anywhere in the codebase and should not be included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a3deffb-e969-42ac-9a4a-ba331d9d0adb
📒 Files selected for processing (8)
.github/scripts/bot-gfi-assign-on-comment.js.github/scripts/bot-gfi-candidate-notification.js.github/scripts/bot-next-issue-recommendation.js.github/scripts/coderabbit_plan_trigger.js.github/scripts/cron-admin-update-spam-list.js.github/scripts/shared/labels.js.github/scripts/shared/labels.test.js.github/workflows/bot-coderabbit-plan-trigger.yml
a172bf9 to
6ad75bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/scripts/shared/labels.test.js (1)
134-177: 🧹 Nitpick | 🔵 TrivialMake env cleanup failure-safe.
Move label env cleanup into a shared helper plus
afterEach; deleting env vars after assertions can leak overrides when a test fails.Proposed refactor
-const { describe, it, beforeEach } = require('node:test'); +const { describe, it, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); +function clearLabelEnv() { + delete process.env.GOOD_FIRST_ISSUE_LABEL; + delete process.env.GOOD_FIRST_ISSUE_CANDIDATE_LABEL; + delete process.env.BEGINNER_LABEL; + delete process.env.INTERMEDIATE_LABEL; + delete process.env.ADVANCED_LABEL; +} + // Helper: force a fresh require() by clearing the module cache. // labels.js reads process.env at load time, so we must reload to test overrides. function freshRequire() { @@ describe('labels.js — environment variable overrides', () => { + beforeEach(clearLabelEnv); + afterEach(clearLabelEnv); + it('overrides GOOD_FIRST_ISSUE_LABEL from env', () => { process.env.GOOD_FIRST_ISSUE_LABEL = 'custom: gfi'; const labels = freshRequire(); assert.equal(labels.GOOD_FIRST_ISSUE_LABEL, 'custom: gfi'); assert.ok(labels.DIFFICULTY_LABELS.includes('custom: gfi')); - delete process.env.GOOD_FIRST_ISSUE_LABEL; });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6336e45-250b-4c51-a07e-32d03c42db1f
📒 Files selected for processing (8)
.github/scripts/bot-gfi-assign-on-comment.js.github/scripts/bot-gfi-candidate-notification.js.github/scripts/bot-next-issue-recommendation.js.github/scripts/coderabbit_plan_trigger.js.github/scripts/cron-admin-update-spam-list.js.github/scripts/shared/labels.js.github/scripts/shared/labels.test.js.github/workflows/bot-coderabbit-plan-trigger.yml
6ad75bc to
0195c7c
Compare
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
0195c7c to
11230e5
Compare
Done ,ready for re-review! |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/bot-coderabbit-plan-trigger.yml (1)
18-22: 🧹 Nitpick | 🔵 TrivialKeep the workflow gate dumb; the difficulty-label allowlist is duplicated again.
This hardcodes the same label list that
.github/scripts/coderabbit_plan_trigger.jsnow derives from.github/scripts/shared/labels.js. The next rename can silently stop the workflow from starting even though the script is already updated. Prefer gating only on event shape/open state here and letting the script own the label allowlist.Suggested simplification
if: > github.event.action == 'labeled' && - github.event.issue.state == 'open' && - contains(fromJson('["skill: beginner","skill: intermediate","skill: advanced"]'), github.event.label.name) + github.event.issue.state == 'open'As per coding guidelines, “Non-trivial logic belongs in dedicated scripts under
.github/scripts/, keeping the workflow YAML focused on orchestration.”.github/scripts/bot-advanced-check.sh (1)
15-15:⚠️ Potential issue | 🟡 MinorValidate
INTERMEDIATE_LABELonce at configuration time.This value now flows into the GraphQL
labels[]filter, the generated label URL, and the user-facing comment body. If it is empty or malformed, the script fails in harder-to-diagnose ways later. Validate it right after the default is applied.Suggested guard
REQUIRED_INTERMEDIATE_COUNT=1 INTERMEDIATE_LABEL="${INTERMEDIATE_LABEL:-skill: intermediate}" +if [[ -z "${INTERMEDIATE_LABEL//[[:space:]]/}" ]] || [[ ! "$INTERMEDIATE_LABEL" =~ ^[A-Za-z0-9][A-Za-z0-9:._/\ -]*$ ]]; then + log "ERROR: INTERMEDIATE_LABEL contains unsupported characters" + exit 1 +fiAs per coding guidelines, “Environment variables should be passed from the YAML, validated early, and defensively quoted.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9bc39008-1b26-4a5b-8ac4-664b9e83167f
📒 Files selected for processing (16)
.github/ISSUE_TEMPLATE/02-beginner-issue.yml.github/ISSUE_TEMPLATE/03-intermediate-issue.yml.github/ISSUE_TEMPLATE/04-advanced-issue.yml.github/scripts/bot-advanced-check.sh.github/scripts/bot-beginner-assign-on-comment.js.github/scripts/bot-gfi-assign-on-comment.js.github/scripts/bot-gfi-candidate-notification.js.github/scripts/bot-intermediate-assignment.js.github/scripts/bot-next-issue-recommendation.js.github/scripts/coderabbit_plan_trigger.js.github/scripts/cron-admin-update-spam-list.js.github/scripts/shared/labels.js.github/scripts/shared/labels.test.js.github/workflows/bot-advanced-check.yml.github/workflows/bot-coderabbit-plan-trigger.yml.github/workflows/bot-intermediate-assignment.yml
|
Approved. Assumed prerequisite: labels will be renamed in GitHub UI (beginner/intermediate/advanced -> skill: beginner/intermediate/advanced) before rollout. |
parvninama
left a comment
parvninama
left a comment
There was a problem hiding this comment.
LGTM 👍
Just one thing — please update (rebase/merge) your branch with the latest base branch so we’re up to date.
There was a problem hiding this comment.
Great refactor to centralize these labels!
I was auditing the workflow triggers and noticed a critical blocker: it looks like .github/workflows/request-triage-review.yml was accidentally left out of this PR.
If we see the line 17 of that file, the workflow is still gating on the old beginner label:
if: ${{ contains(github.event.pull_request.labels.*.name, 'Good First Issue') || contains(github.event.pull_request.labels..name, 'beginner') }}Since the issue templates in this PR now apply skill: beginner, the triage workflow will silently stop triggering for new beginner PRs once this merges.
Could you update that condition to check for skill: beginner (or both, during the rollout transition)? Everything else looks really solid, well done !
11230e5 to
8b07af9
Compare
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
|
thank you everyone for the reviews!! @exploreriii please let me know if any more changes required! |
exploreriii
added
step: 2nd
github-actions
Bot
added
skill: intermediate
|
When you have a moment, please update your branch. Thank you! |
|
@aceppaluni Done! |
Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
7 participants
Description:
Centralize and rename difficulty labels from
beginner/intermediate/advancedtoskill: beginner/skill: intermediate/skill: advancedso future label renames only require a single-file change..github/scripts/shared/labels.js) as single source of truthif:conditions andenv:blocksmigrate-skill-labels.sh) for existing open issuesisSafeLabelvalidator for labels containing colons/spaces (keepsisSafeSearchTokenstrict for owner/repo/username)gh apiarray syntax (labels[]=) for GraphQL variable bindingRelated issue(s):
Fixes #2131
Notes for reviewer:
Tested on my fork — labels, templates, and workflow triggers all work as expected: https://github.com/DeathGun44/hiero-sdk-python/issues
Checklist