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

Add reader MatchingNode results and a signal to stop reading #66

Merged
merged 1 commit into from
Jan 14, 2024

Conversation

veewee
Copy link
Owner

@veewee veewee commented Jan 6, 2024

Q A
Type improvement
BC Break yes
Fixed issues #58

Summary

❗ This is a BC break and should be released in a new major version ❗

use VeeWee\Xml\Dom\Configurator;
use VeeWee\Xml\Reader\Signal;
use function VeeWee\Xml\Reader\Matcher\element_name;
use function VeeWee\Xml\Reader\Matcher\sequence;


$xml = <<<XML
    <container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
      <services>
        <service id="service_container" class="Symfony\Component\DependencyInjection\ContainerInterface" public="true" synthetic="true"/>
        <service id="kernel" class="TicketSwap\Kernel" public="true" synthetic="true" autoconfigure="true">
          <tag xmlns="http://symfony.com/schema/dic/tag-1" name="controller.service_arguments">1</tag>
          <tag xmlns="http://symfony.com/schema/dic/tag-2"  name="routing.route_loader">2</tag>
        </service>
      </services>
    </container>
XML;


$reader = VeeWee\Xml\Reader\Reader::fromXmlString($xml);
$signal = new Signal();

foreach ($reader->provide(element_name('tag'), $signal) as $tag) {
    // New result of the reader is now a DTO that contains both the XML and node sequence.
    var_dump(
    // This is the previous matched XML `string`
        $tag->xml(),
        // Contains a match function so that you can run a secondary matcher on the result.
        // Can be handy if you are matching on multiple paths in one read for example.
        $tag->matches(sequence(
                element_name('container'),
                element_name('services'),
                element_name('service'),
                element_name('tag')
        )),
        // You can now directly pull the xml into a DOM Document with or without a DOM configurator.
        $tag->intoDocument(Configurator\canonicalize()),
        // Or decode it directly using xml_decode:
        $tag->decode(Configurator\canonicalize()),
        // You can get access to the current matching NodeSequence data:
        $tag->nodeSequence(),
    );

    // Stops reading on first `tag` match.
    $signal->stop();
}

@veewee veewee added enhancement New feature or request Breaking change labels Jan 6, 2024
@veewee veewee force-pushed the reader-improvements branch 3 times, most recently from 84a64db to e865c40 Compare January 6, 2024 14:04
@veewee veewee force-pushed the reader-improvements branch 3 times, most recently from d41dfe5 to fab0bf1 Compare January 7, 2024 15:30
@veewee
Copy link
Owner Author

veewee commented Jan 7, 2024

@Sammyjo20 Any remarks about this PR? It's gonna be tagged as 3.0 eventually.
(need to do some cleanup and check if I can introduce other Breaking changes in other components as well)

@Sammyjo20
Copy link

Hey @veewee I really like it, instead of the XML string being returned, having an object that allows you to perform further things sounds great. Did you manage to do that while keeping memory low?

My only thought so far is on the BC. You could introduce a separate method on the reader to perform this newer kind of match and keep the old one as the simple one, but in all honesty I quite like the change and it keeps it simple.

@Sammyjo20
Copy link

I wonder if this will make some of XML wrangler's logic simpler too, especially around the idea I had of being able to continue reading from an element.

@veewee
Copy link
Owner Author

veewee commented Jan 8, 2024

Did you manage to do that while keeping memory low?

Yes, memory consumption should be similar to what we had before. It still works the same way internally, but it now returns more detailed information (we already had in the reader's internals). This repo comes with stress tests that ensures that there are no memory leaks in the reader and writer btw.

My only thought so far is on the BC. You could introduce a separate method on the reader to perform this newer kind of match and keep the old one as the simple one, but in all honesty I quite like the change and it keeps it simple.

I rather tag a new major than providing 2 separate methods so that I can provide 1 good way of using the tool. That's what major versions are for IMO : telling people things could be done in a better way.

I wonder if this will make some of XML wrangler's logic simpler too, especially around the idea I had of being able to continue reading from an element.

There are some shortcut functions for transforming into a document or decoding. So that part would become slightly easier.
Can't recal that idea you are talking about anymore. Can you elaborate on what you want to achieve exactly?

@Sammyjo20
Copy link

Sammyjo20 commented Jan 8, 2024

Sounds great 👍

Can you elaborate on what you want to achieve exactly?

I wanted to make it easier to chain reading on from an element. So you could do something like:

[$elementOne] = $reader->element('food')->get();

$elementOne->reader('something-else')->get();

I wonder if there's a way to chain provide on one of the element objects that your reader produces? 🤔 (To keep memory consumption low)

@veewee
Copy link
Owner Author

veewee commented Jan 8, 2024

I think that should already be possible as long as you store the XML string in the Element class.
That way, you can construct a new Reader from XML string and iterate on the provided matches from that point.

It will be faster than rereading the whole XML document. Yet it won't have a positive impact on memory cause the XML was already read in memory and you are performing a second read on top of that xml (so more memory). But again: in comparison with re-reading the whole XML document, it should have a possitive impact.

@Sammyjo20
Copy link

That is what I have built as a PoC so far 😀 Cool I might carry on with that one then. As always nice work on this PR - and looking to v3!

@veewee veewee changed the title [WIP] Add reader MatchingNode results and a signal to stop reading Add reader MatchingNode results and a signal to stop reading Jan 8, 2024
@veewee veewee changed the base branch from main to 3.x January 14, 2024 12:26
@veewee veewee merged commit 1770824 into 3.x Jan 14, 2024
30 checks passed
@veewee veewee deleted the reader-improvements branch January 29, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants