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

Configuration Helper get(Index|View) functions can return NULL. #260

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

ekes
Copy link
Member

@ekes ekes commented Jan 18, 2023

All calls to these methods already are actually checking if a value is returned. It makes sense that there are times there isn't a view or index (import for example). There are other checks later that it shoud operate on these indexes and views.

Fixes #259

Longer description #259 (comment)

All calls to these methods already are actually checking if a value is
returned. It makes sense that there are times there isn't a view or
index (import for example). There are other checks later that it shoud
operate on these indexes and views.

Fixes #259
@ekes
Copy link
Member Author

ekes commented Jan 18, 2023

@chriswales95 this work for you?

@chriswales95 chriswales95 self-requested a review January 18, 2023 15:51
@ekes ekes changed the title Get functions can return NULL. Configuarion Helper get(Index|View) functions can return NULL. Jan 18, 2023
@ekes ekes changed the title Configuarion Helper get(Index|View) functions can return NULL. Configuration Helper get(Index|View) functions can return NULL. Jan 18, 2023
@chriswales95
Copy link
Member

@ekes Looks good to me! I had the same solution, but was hesitant to suggest not knowing the code-base as well. Glad to give my stamp of approval.

@ekes
Copy link
Member Author

ekes commented Jan 18, 2023

So. This is a class that doesn't have an interface, it's not likely to be called from code externally. But I does change the signature of the function that a public method of a class that's not marked as internal.

Thoughts maybe from people using it @Adnan-cds @andybroomfield @budda This one should really be a minor version bump?

Copy link
Member

@finnlewis finnlewis left a comment

Choose a reason for hiding this comment

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

This certainly fixes the original bug for me.
I can import config that enables localgov_directories_venue without error.
Thanks @ekes!

@andybroomfield
Copy link
Contributor

These methods don't appear any use at BHCC

@finnlewis finnlewis merged commit 441c92c into 2.x Jan 19, 2023
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.

Error importing config when enabling locagov_directories_venue
4 participants