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

Rt 11724 is it really a bug ? #2

Merged
merged 5 commits into from Jan 20, 2016

Conversation

Projects
None yet
2 participants
@sdeseille
Contributor

sdeseille commented Jan 19, 2016

Hello again.

I have the felling it is not really a bug.
In fact $xp->find('/xml/tag') call return an 'XML::XPath::NodeSet' object.

So in order to test it we have to invoke "string_value()" method.

I added "99rt_11724.t" in t directory.
Can you take a look to say me if you agree with my understanding about this RT ?

Regards

sdeseille

sdeseille added some commits Jan 19, 2016

Create test file with piece of code in RT 11724
RT 11724 have piece of code describing bug encountered. I take it and
transform it as standard test file.
Add some tests to understand it is not really a bug
It seem it is not a bug but a normal behaviour.
@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Jan 19, 2016

Owner

Hi,

You are going in the right direction, well done :-)

As the ticket RT# 11724 says the package XML::XPath::NodeSet is not overloading "eq", "ne" etc. So, for example, in order to enable the "eq" overloading, you could just change one line as below:

diff --git a/lib/XML/XPath/NodeSet.pm b/lib/XML/XPath/NodeSet.pm
index 7a86d52..8520e3e 100644
--- a/lib/XML/XPath/NodeSet.pm
+++ b/lib/XML/XPath/NodeSet.pm
@@ -8,6 +8,7 @@ use XML::XPath::Boolean;

use overload
            '""' => \&to_literal,
 +                'eq' => \&string_value,
                 'bool' => \&to_boolean,
       ;

Here is the test script to verify the above change.

use Test::More;
use XML::XPath;

my $sample = qq{ <xml><tag>FOO</tag> </xml> };
my $xp = XML::XPath->new(xml=>$sample);
my $nodelist = $xp->find('/xml/tag');

ok($nodelist eq 'FOO');

done_testing();

Best Regards,
Mohammad S Anwar

Owner

manwar commented Jan 19, 2016

Hi,

You are going in the right direction, well done :-)

As the ticket RT# 11724 says the package XML::XPath::NodeSet is not overloading "eq", "ne" etc. So, for example, in order to enable the "eq" overloading, you could just change one line as below:

diff --git a/lib/XML/XPath/NodeSet.pm b/lib/XML/XPath/NodeSet.pm
index 7a86d52..8520e3e 100644
--- a/lib/XML/XPath/NodeSet.pm
+++ b/lib/XML/XPath/NodeSet.pm
@@ -8,6 +8,7 @@ use XML::XPath::Boolean;

use overload
            '""' => \&to_literal,
 +                'eq' => \&string_value,
                 'bool' => \&to_boolean,
       ;

Here is the test script to verify the above change.

use Test::More;
use XML::XPath;

my $sample = qq{ <xml><tag>FOO</tag> </xml> };
my $xp = XML::XPath->new(xml=>$sample);
my $nodelist = $xp->find('/xml/tag');

ok($nodelist eq 'FOO');

done_testing();

Best Regards,
Mohammad S Anwar

sdeseille added some commits Jan 19, 2016

overloading eq and ne operator with the help of Manwar
Manwar open my eyes about the possibility to overload 'eq' and 'ne'
operator. I modified "NodeSet.pm" and updated my testfile.
Added string and number comparison operator test
I added all string and number comparison operators in overload.
Added testfile dedicated to number comparison operators.
@sdeseille

This comment has been minimized.

Show comment
Hide comment
@sdeseille

sdeseille Jan 19, 2016

Contributor

Hello

Thank you for your advice. I didn't know that was possible to do that.

I added all string and number operators.
I added a new testfile dedicated to number comparison operators.

regards

sdeseille

Contributor

sdeseille commented Jan 19, 2016

Hello

Thank you for your advice. I didn't know that was possible to do that.

I added all string and number operators.
I added a new testfile dedicated to number comparison operators.

regards

sdeseille

manwar added a commit that referenced this pull request Jan 20, 2016

Merge pull request #2 from sdeseille/RT_11724
Rt 11724 is it really a bug ?

@manwar manwar merged commit 2d5dbbb into manwar:master Jan 20, 2016

@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Jan 20, 2016

Owner

Thanks.

Owner

manwar commented Jan 20, 2016

Thanks.

manwar added a commit that referenced this pull request Jan 20, 2016

- Merged in GitHub PR #2, thanks to sdeseille.
- Merged t/45overloading_number_operator.t and t/99rt_11724.t into t/45cmp_nodeset.t

@sdeseille sdeseille deleted the sdeseille:RT_11724 branch Jan 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment