Skip to content

Refactor permissions parsing from compiler.go to permissions.go#2111

Closed
Copilot wants to merge 4 commits intomainfrom
copilot/refactor-permissions-parsing-code
Closed

Refactor permissions parsing from compiler.go to permissions.go#2111
Copilot wants to merge 4 commits intomainfrom
copilot/refactor-permissions-parsing-code

Conversation

Copy link
Contributor

Copilot AI commented Oct 22, 2025

  • Analyze the duplicated permissions parsing logic between compiler.go and permissions.go
  • Move extractPermissions logic from compiler.go to permissions.go (as ExtractPermissionsYAML)
  • Move convertKeyToPermissionScope logic from compiler.go to permissions.go (as ConvertKeyToPermissionScope)
  • Update compiler.go to use the refactored permissions.go functions
  • Run tests to ensure "all: read" feature still works
  • Verify all tests pass (unit + integration)
  • Format code and run linter
  • Verify build succeeds
  • Simplify permissions.go by removing duplicate validation logic
  • Enhance ExtractPermissionsYAML to handle all permission cases

Summary

Successfully refactored and simplified permissions parsing code to centralize all permissions logic in permissions.go:

Refactoring Changes:

  1. Created parsePermissionsMap() helper method - Extracted duplicate "all: read" validation logic into a shared private method used by both parse() and NewPermissionsParserFromValue()
  2. Enhanced ExtractPermissionsYAML() to handle ALL permission cases - Now processes:
    • Shorthand permissions (read-all, write-all, read, write, none)
    • "all: read" expansion with explicit overrides
    • Regular permission maps (e.g., {contents: read, issues: write})
  3. Simplified extractPermissions() in compiler.go - Now fully delegates to ExtractPermissionsYAML() from permissions.go with minimal fallback logic
  4. Eliminated ~100 lines of duplicate code across both files

Overall Impact:

  • Complete permissions parsing logic now in permissions.go - Single source of truth for all permissions operations
  • Compiler.go uses parser functions from permissions.go - All permissions parsing delegates to permissions.go functions
  • New/enhanced shared functions: ConvertKeyToPermissionScope(), ExtractPermissionsYAML(), parsePermissionsMap()

Test Results:

  • ✅ All permissions tests pass
  • ✅ All unit tests pass
  • ✅ Code formatted successfully
  • ✅ Build succeeds
Original prompt

Review the permissions parsing code added in compiler.go in 9f93395 and update it with existing parsing code in permissions.go. The permissions parsing should not be in compiler.go Make sure the "all: read" feature is supported.

Make sure the test still pass, do not modify tests.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor permissions parsing to remove from compiler.go Refactor permissions parsing from compiler.go to permissions.go Oct 22, 2025
Copilot AI requested a review from pelikhan October 22, 2025 09:07
@pelikhan
Copy link
Contributor

@copilot review all code in permissions.go and simplify/remove duplicate.

Make sure the tests are passing. Do not modify existing tests.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot review all code in permissions.go and simplify/remove duplicate.

Make sure the tests are passing. Do not modify existing tests.

Reviewed and simplified permissions.go. Removed ~30 additional lines of duplicate code by:

  1. Created parsePermissionsMap() helper method to share "all: read" validation logic between parse() and NewPermissionsParserFromValue()
  2. Simplified ExtractPermissionsYAML() to reuse NewPermissionsParserFromValue() for validation

All tests passing. Changes in commit a0c5ff9.

Copy link
Contributor

@pelikhan pelikhan left a comment

Choose a reason for hiding this comment

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

@copilot review compiler.go and move/refactor permissions parsing code into permissions.go

When possible, use the parser functions defined in permissions.go

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 22, 2025

@copilot review compiler.go and move/refactor permissions parsing code into permissions.go

When possible, use the parser functions defined in permissions.go

Enhanced ExtractPermissionsYAML() in permissions.go to handle all permission cases (shorthand, "all: read" expansion, and regular maps). The extractPermissions() function in compiler.go now fully delegates to permissions.go parser functions.

Changes in commit 5cce6a8.

Copilot AI requested a review from pelikhan October 22, 2025 09:32
@pelikhan pelikhan closed this Oct 22, 2025
@pelikhan pelikhan deleted the copilot/refactor-permissions-parsing-code branch October 23, 2025 21:23
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