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
Sitemap index: stop URL at closing </loc> #227
Sitemap index: stop URL at closing </loc> #227
Conversation
- at start of a <loc> element auto-close any unclosed <sitemap> element and add the sitemap if there is a valid URL from the previous <loc> element
maybeAddSiteMap(); | ||
return; | ||
} | ||
loc = new StringBuilder(); |
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.
Wouldn't you want to always reset loc
, regardless of whether localName
is loc
or sitemap
?
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.
If the start element holds an attribute (e.g., <lastmod>
) the content of loc
is still required. An example:
<sitemapindex>
<sitemap>
<loc>http://www.example.com/sitemap1.xml.gz</loc>
<lastmod>2004-10-01T18:23:17+00:00</lastmod>
<sitemap>
<loc>...
At second opening <sitemap>
we can be sure that the previous sitemap is finished. But at the opening <lastmod>
we need to wait for the modification date of the sitemap before we can call maybeAddSiteMap()
.
return; | ||
} | ||
loc = new StringBuilder(); | ||
if ("url".equals(localName)) { |
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.
Would this code ever get triggered, given the && ("loc".equals(localName) || "sitemap".equals(localName)
clause above?
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.
Also, is there a way to unit test the reason why we want to do this reset of attributes?
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.
get triggered
Right, my fault. Should be "sitemap".equals(localName)
- a copy-paste error when copying from XMLHandler. I'll also add a unit test regarding resetting the attributes.
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.
A few questions...
- bug fix: <sitemap> element is closed, not <url> - add unit test to cover resetting of attributes
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.
lgtm
Thanks, @kkrugler! |
(fixes #213)
At start of a
<loc>
element auto-close any "unclosed"<sitemap>
element and add the sitemap if there is a valid URL from the previous<loc>
element.