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

Update update_cached_concordances command to comply with new spec #1333

Merged
merged 6 commits into from
Feb 27, 2024

Conversation

jacobdgm
Copy link
Contributor

This PR updates our update_cached_concordances command to output a file matching the updated specification (see #1323). In doing so, it fixes #1323.

The tests have also been updated to reflect the new spec.

@jacobdgm
Copy link
Contributor Author

jacobdgm commented Feb 27, 2024

I spent a few minutes looking into whether we could rename "feast__name" -> "feast" etc. within get_concordances when calculating values by using .annotate() , without having to have a separate make_chant_dict function that gets called in the list comprehension. But initial results suggest it will be tricky to create "chantlink" and "srclink" on the database level. Considering the function already takes but a couple of seconds to run, I'm thinking it's not going to be worth the trouble to apply this particular optimization. Or do any of you tagged as reviewers think this is worth looking into?

@dchiller
Copy link
Contributor

Were you using concat for srclink and chantlink? https://docs.djangoproject.com/en/5.0/ref/models/database-functions/#concat I think that would be the way to do what you are referring to.

I don't feel like this is necessary unless you want to.

@jacobdgm
Copy link
Contributor Author

jacobdgm commented Feb 27, 2024

Were you using concat for srclink and chantlink? https://docs.djangoproject.com/en/5.0/ref/models/database-functions/#concat I think that would be the way to do what you are referring to.

I don't feel like this is necessary unless you want to.

This is it, yes!

Maybe I'll get around to refactoring this once we've finished rolling it out as it is. For now, though, I think we're fine as-is.

@jacobdgm jacobdgm merged commit deaea3c into DDMAL:develop Feb 27, 2024
@jacobdgm jacobdgm deleted the 1323-concordances branch February 27, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We need to update our concordances export API for new specification
3 participants