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

#4190: Search API integration - Property label and description #4191

Merged

Conversation

stefan-korn
Copy link
Contributor

@stefan-korn stefan-korn commented Jun 3, 2024

fixes #4190

@stefan-korn
Copy link
Contributor Author

One remark: I am not sure if the code would ever get here: https://github.com/GetDKAN/dkan/blob/2.x/modules/metastore/modules/metastore_search/src/ComplexData/Dataset.php#L77
With default DKAN schema it does not get there I suppose, if it would get there there might be a problem with this MR.

@dafeder
Copy link
Member

dafeder commented Jun 3, 2024

One remark: I am not sure if the code would ever get here: https://github.com/GetDKAN/dkan/blob/2.x/modules/metastore/modules/metastore_search/src/ComplexData/Dataset.php#L77 With default DKAN schema it does not get there I suppose, if it would get there there might be a problem with this MR.

@stefan-korn yeah tbh I understand that final else looking at it. There are no other types than object or array that make sense here, the conditional should either not be there or throw an exception.

@dafeder dafeder self-assigned this Jun 3, 2024
@dafeder dafeder self-requested a review June 3, 2024 18:29
Copy link
Member

@dafeder dafeder 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 @stefan-korn! What an improvement.

I think I'll add some test coverage for this and update the default search config in another PR but IMO this can be merged as is since technically the code is "covered" by a test just not by any specific assertions.

@dafeder dafeder merged commit 33c8ca0 into GetDKAN:2.x Jun 11, 2024
10 checks passed
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.

Search API integration - Property label and description
2 participants