-
Notifications
You must be signed in to change notification settings - Fork 26
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
Breaking change: component identifier format #1478
Conversation
9ed8336
to
59aa0a0
Compare
Here is the text of the wiki that I wrote -- to publish when we do a release [edited based on Sean's work]
|
59aa0a0
to
ab21610
Compare
9817f69
to
3de2de5
Compare
This breaking change closes #1318
3de2de5
to
ddc57c1
Compare
I also wanted to include this from @randalldfloyd from Slack since it gives good context for the breaking change: -- If someone pulls in this breaking change, they will have to run a reindex to get the new underscores into their documents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great, @marlo-longley. I got thinking about the complexity of having to override that Parent.global_id
method in addition to changing the format in the traject config and thought it'd be ideal to only have to specify this setting in one place.
I branched off your branch (see identifier-format-no-global-id) and made one modest commit that looks to me like it works:
1035486
Curious what you think.
@seanaery thanks so much for digging into this. Your work is a big improvement in my opinion! For reducing codebase complexity and also for implementers using this new setting. I tested your branch and all looks good. I am fine if you wanted to push your commit to this branch/PR, or PR your branch -- not sure the best way. |
…red in one place. Advances #1318 - Capture the concatenated/formatted IDs at indexing time, in parent_ids_ssim array - Use that data instead of global_id
@marlo-longley Sounds great -- in that case I will cherry-pick my commit into this branch and push it back up so this PR remains the one under review (it'll just have three contributors now). |
I branched off Chris's work on this in a branch from last year. Closes #1318
This PR will have 2 parts (will undraft after both are in):
I also drafted text for a wiki entry on how to use the new traject setting, and how to customize in order to keep the old format after this breaking change.