Skip to content

Brief refactoring in response to Copilot/Claude code review - #83

Open
rshiggin wants to merge 1 commit into
mainfrom
arc-170-repo-id-requestable-bug-refactor
Open

Brief refactoring in response to Copilot/Claude code review#83
rshiggin wants to merge 1 commit into
mainfrom
arc-170-repo-id-requestable-bug-refactor

Conversation

@rshiggin

@rshiggin rshiggin commented Aug 25, 2026

Copy link
Copy Markdown
Member

@rshiggin
rshiggin marked this pull request as ready for review August 25, 2026 20:45
@rshiggin
rshiggin requested a review from ssciolla August 25, 2026 20:45

@ssciolla ssciolla left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Mildly suggesting that we use a more specific error type.

Comment on lines +72 to +76
raise "REPOSITORY_ID is not set; cannot index repository fields" if slug.blank?
repository = Arclight::Repository.find_by(slug: slug)
if repository.nil?
raise "No repository found for slug #{slug.inspect} — check REPOSITORY_ID " \
"against the slugs defined in config/repositories.yml"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could define a custom error type like TrajectEADIndexError and raise that specifically. What we're doing now will result in RuntimeErrors I think. E.g.

class TrajectEADIndexError < RuntimeError
end

# in your code
raise TrajectEADIndexError, "REPOSITORY_ID is not set..." if slug.blank?

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