chore: Rename Index -> AutoIndexJob#63955
Conversation
aa87fef to
8742bac
Compare
|
Could you please rebase this one before I take a look? |
8742bac to
853f94c
Compare
cad1313 to
5c9a365
Compare
kritzcreek
left a comment
There was a problem hiding this comment.
I agree the existing name is confusing. Not a fun amount of churn tough...
There was a problem hiding this comment.
IndexLoaderFactory -> AutoIndexJobLoaderFactory
(starting to approach peak Java)
There was a problem hiding this comment.
(starting to approach peak Java)
The number of design patterns being used is unchanged (dataloader pattern and factory pattern) 😛
I'm surprised we're using factories though, I wonder if that's actually necessary/can be simplified, given that we don't have inheritance/struct embedding here.
There was a problem hiding this comment.
Should we start the process of changing the GraphQL API as well? (new field + deprecation of old one)
Otherwise I fear we're just not going to do it and have different names in the code from the API
There was a problem hiding this comment.
Updating the GraphQL API directly is a bit tricky because it currently mushes the two concepts of an "upload" and an "auto-indexing job" into a single type. But yes, we should fix that, I'll file a follow-up issue. I've also been thinking about renaming the tables.
5c9a365 to
778ac73
Compare
The mocks and test code are inflating the line count by too much, the actual delta is currently around |
kritzcreek
left a comment
There was a problem hiding this comment.
Marked a couple more places I spotted. Looks good in general!
|
Addressed all pending comments, thanks for taking a look. |
IndexJobinautoindex/config/types.gotoAutoIndexJobSpecTest plan
Covered by existing tests
Changelog