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

Handle error in event index pipeline when a term is blank #237

Merged

Conversation

conantp
Copy link
Contributor

@conantp conantp commented Jun 4, 2023

Link to Relevant Issue

This pull request resolves an issue I identified and shared on Slack.

Description of Changes

This PR adds an if statement to generate_event_index_pipeline.py. When a sentence contains a "-" character, it winds up being evaluated as a "" value, which then causes an error on line 254:

target_term_index = terms.index(closest_term)

This causes the entire event index operation to fail, and the only option to restore index functionality is to remove the event that caused the issue. This PR allows event index to continue, skipping that term.

@codecov
Copy link

codecov bot commented Jun 4, 2023

Codecov Report

Merging #237 (04c47b9) into main (703d7f1) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
- Coverage   72.12%   72.11%   -0.02%     
==========================================
  Files          50       50              
  Lines        3376     3378       +2     
==========================================
+ Hits         2435     2436       +1     
- Misses        941      942       +1     
Impacted Files Coverage Δ
..._backend/pipeline/generate_event_index_pipeline.py 96.68% <50.00%> (-0.63%) ⬇️

Copy link
Member

@evamaxfield evamaxfield left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

I will push a new release in a bit. Unfortunately lost my two factor auth app for pypi so can't deploy new stuff right now hahaha. Already in the process for account recovery.

@evamaxfield
Copy link
Member

Oh woops, can you run just lint / fix the lint error please?

@evamaxfield
Copy link
Member

Will fix lint after merge! Thanks!

@evamaxfield evamaxfield merged commit 58a9fd7 into CouncilDataProject:main Jun 14, 2023
6 of 9 checks passed
@conantp
Copy link
Contributor Author

conantp commented Jun 14, 2023

Sorry I missed this last week! Since it's already merged, I will just remember to lint prior to PR in the future!

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.

None yet

2 participants