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

Extract constant_name method #3136

Merged
merged 5 commits into from
Feb 5, 2025
Merged

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Feb 4, 2025

This came up while pairing on #3031

@vinistock you initially suggested extracting to GlobalState but it's also used in ReferenceFinder, so I think having on Index makes more sense?

Copy link

graphite-app bot commented Feb 4, 2025

How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@andyw8 andyw8 force-pushed the andyw8/extract-constant-name-method branch from 60a4ade to 3ef4123 Compare February 4, 2025 17:19
@andyw8 andyw8 marked this pull request as ready for review February 4, 2025 17:20
@andyw8 andyw8 requested a review from a team as a code owner February 4, 2025 17:20
@andyw8 andyw8 added chore Chore task server This pull request should be included in the server gem's release notes labels Feb 4, 2025
@andyw8
Copy link
Contributor Author

andyw8 commented Feb 4, 2025

@vinistock I've removed the methods that were solely for delegating, but I had intentionally done it that way as I think it improves readability, since there is less noise around those calls.

@vinistock
Copy link
Member

Ah, actually, let's keep only the one in common.rb. Otherwise, this is a breaking change as the Rails add-on depends on constant_name.

But let's leave it just to avoid the breaking change, we can use Index.constant_name directly in our implementation.

@andyw8 andyw8 requested a review from vinistock February 4, 2025 20:51
@andyw8 andyw8 merged commit 6f99b6e into main Feb 5, 2025
43 checks passed
@andyw8 andyw8 deleted the andyw8/extract-constant-name-method branch February 5, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Chore task server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants