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.

Sign up for GitHub

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

Jump to bottom

Raise error when no value is present to add to FacetTree #93

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Raise error when no value is present to add to FacetTree #93

wants to merge 3 commits into from
+74 −10

Conversation

maxkadel
Copy link

  • This makes the gem more resilient to bad data

Closes #91

@maxkadel
Only add values to FacetTree when present
3740416 
- This makes the gem more resilient to bad data

Closes sul-dlss#91
@jcoyne
Copy link
Contributor

jcoyne commented Feb 10, 2025

Doesn't this silently hide bad data? Should we raise an error when invalid data is encountered so that we can fix it?

@maxkadel
Copy link
Author

Doesn't this silently hide bad data? Should we raise an error when invalid data is encountered so that we can fix it?

Are you thinking a raise and a rescue & log, or just a raise to make it easier for consumers to debug?

@jcoyne
Copy link
Contributor

jcoyne commented Feb 10, 2025

@maxkadel my initial thought would be to raise some sort of "bad data, expected xxxx, found yyy". But I suppose a log message is fine too. The former is more likely to be spotted on prod as we monitor exceptions, but not necessarily log messages.

@maxkadel
Copy link
Author

@jcoyne - I've tried to implement what you suggest, let me know what you think.

@maxkadel maxkadel changed the title Only add values to FacetTree when present Raise error when no value is present to add to FacetTree Feb 11, 2025
@jcoyne
Copy link
Contributor

jcoyne commented Feb 12, 2025

Thanks @maxkadel looks great.

@maxkadel
Remove old versions of rails from test matrix
13a2150 
- See more context in Blacklight issue projectblacklight/blacklight#3493
- Also added 8.x release of blacklight to matrix, since that is what folks are moving towards (or have moved to)
- Updated enginecart config based on output
@maxkadel maxkadel mentioned this pull request Feb 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcoyne jcoyne jcoyne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Be more resilient to bad data
2 participants
@maxkadel @jcoyne