-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fix search index for entries without parent book #160
Conversation
This commit enhances the search index generation process by providing more meaningful descriptions for entries that lack a parent <book> element. Additionally, refactors writeJsonIndex() into smaller methods. Fixes php#159
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really dealt much with the indexing part of PhD, maybe @haszi knows more about.
phpdotnet/phd/Package/PHP/Web.php
Outdated
|
||
// isset() to guard against undefined array keys, either for root | ||
// elements (no parent) or in case the index structure is broken. | ||
while (isset($this->indexes[$id])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use array_key_exists()
as isset()
will return false
if the key exists, but the associated value is null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have one comment and a nit, but looks good in general. One thing I would really like to see is some tests for this though.
44052a0
to
4ba7d78
Compare
Also, updated the doc block with additional context.
It appears there aren’t any tests yet for the To keep things moving, I can create an issue specifically for adding these tests in a follow-up PR. This way, we can proceed with the merge now, and we’ll have a clear plan to cover the tests soon after. Does that work for you? |
I'm OK with that as long as the tests for the two new methods are added soonish. :-) |
Fixes #159
This commit enhances the search index generation process by providing more meaningful subtitles (descriptions) for entries that lack a parent
<book>
element.View the entire table