search

Cumulus / Syndic

RSS and Atom feed parsing
MIT License
34 stars 13 forks source link

Implement xml:base #52

Closed Chris00 closed 9 years ago

Chris00 commented 9 years ago

See also https://github.com/ocaml/platform-blog/issues/12 for the reason prompting this change.

Chris00 commented 9 years ago

Any opinion on these commits?

Chris00 commented 9 years ago

@dsheets any opinion on the way xml:base is handled here?

dsheets commented 9 years ago

This looks reasonable. I don't see where the whole thing fits together and the propagation of xml:base happens but I trust you've taken care of that. Every parsed XML tag can introduce a new (relative) xml:base scope including the root element and the top level data structures (sources or events or something?? I didn't see where these got parsed and passed down to the lower layers).

Hope this helped. I don't have any more comments about the PR.

Chris00 commented 9 years ago

I don't see where [...] the propagation of xml:base happens

The function generate_catcher takes lists of callbacks, each with an xmlbase: Uri.t option argument, to parse sub-nodes (and attributes). generate_catcher makes sure the xml:base it receives is given (appropriately modified) to callbacks.

Chris00 commented 9 years ago

I now get xmlbase before parsing the other attributes and sub-elements.

Chris00 commented 9 years ago

@dsheets Thanks a lot for your careful reading of my changes!

Chris00 commented 9 years ago

@dinosaure I intend to merge it later today unless you comment.

dinosaure commented 9 years ago

Sorry for my no response but I have big problem with my only computer (in China) for a long time. I saw a piece of PR and I trust you. Sorry.

Chris00 commented 9 years ago

Sorry for you. I also fixed the <content:encoded> which is required for the OCaml web site. How about to cut a new release?