rpgoldman / xmls

Simple, lightweight XML library for Common Lisp
Other
20 stars 5 forks source link

Wip/extract path #2

Closed a-daniel-eliason closed 2 years ago

a-daniel-eliason commented 4 years ago

Hi Robert, I finally got my act together and put a first set of changes into XMLS. I set up some tests both for integration and also for example usage of EXTRACT-PATH. I've always used the list representation, so I wonder if next I should consider: 1) how to adapt the concept to the structure based node representation. 2) whether to extend the tag attribute match to allow more than one tag. Any suggestion? Thanks! -Daniel E.

a-daniel-eliason commented 4 years ago

Oh, another thing, whether namespaces should be handled other than ignoring in the XML? Should the search path accept namespaced items?

rpgoldman commented 4 years ago

Thanks, @a-daniel-eliason -- Sorry it's taken me so long to get around to reviewing this. EXTRACT-PATH looks good, and it would be nice to make it polymorphic so that it handles the structures. I'll see what I can suggest. Now looking at the namespace issue.

rpgoldman commented 4 years ago

@a-daniel-eliason Added a quick patch that extends this to the structures that one can get from xmls:parse, as a pull request to your pull request.

I'm not exactly sure what you mean about namespaces. The problem is that, to the extent I remember, XMLS essentially looks at the NS, but doesn't pay any attention to its semantics or scope them correctly.

a-daniel-eliason commented 4 years ago

Hi @rpgoldman, thanks for the feedback! I took in your pull request to start working with it. No problem about timing, I've not spent much time on side work either.

rpgoldman commented 4 years ago

So this new version will actually return nodes when given nodes, instead of always giving back s-expressions whether given nodes or s-expressions, right?

rpgoldman commented 4 years ago

What conclusions did you come to about namespaces?

a-daniel-eliason commented 4 years ago

Hi Robert - Yes, your unit test gave me the idea: if someone is working in NODEs, they probably don't want lists returned. I realized the conversion in the code is just changing the references to the node of the parse tree, straight forward. I also see there are already PARSE and PARSE-LIST, so I followed that naming. What I wonder now is whether the KEY-LIST argument syntax should somehow work with NODEs differently, like maybe have the :ATTRS keyword in front of the optional attributes spec. About namespaces, if they're not important, then I'm happy with ignoring them as I have. Or the KEY-LIST syntax could optionally work them in with ( . ), or respectively ( :NS ), or something. Also I wonder about should I allow matching on more than one attribute/value pair.

a-daniel-eliason commented 4 years ago

I got another idea - I realized rather than making two copies of EXTRACT-PATH, it would make more sense to push the node type handling down into the XMLREP helpers and use them in EXTRACT-PATH. I think I was able to work with the argument protocols so as to avoid changing any working code that depends on the XMLREP helpers, but I extended them to operate on either node kind. Now, back to having one EXTRACT-PATH, I can extend the path syntax without having to duplicate effort.

a-daniel-eliason commented 4 years ago

I think this is the end for EXTRACT-PATH, at least for the time being:

I might add some more to the 5am tests to cover the multiple attribute matching. Otherwise I think I've gone where I wanted to with EXTRACT-PATH. Always open to suggestion, of course, if there's something else makes sense to add.

a-daniel-eliason commented 2 years ago

Hi Robert, wondering why we never worked out a merge on this? Did you like my mods to the XMLREP accessors? Best regards!

rpgoldman commented 2 years ago

Hi. Sorry I lost track of this. Would you mind rebasing, then I will look it over one more time and make a new release. Also I will check and make sure that Quicklisp gets an update.

a-daniel-eliason commented 2 years ago

Hi Robert, I rebased changes to the current master, everything went easily and the 5AM tests run fine. I also reviewed my changes since it's been a while since I looked them over, and happily I'm still happy with them. Let me know if there's anything else you'd like for working out a merge. Thanks!

a-daniel-eliason commented 2 years ago

Thanks for the merge, Robert! Nice to have it by Quicklisp!