Prefixed attribute accessors #726

Closed
wants to merge 7 commits into
from

Projects

None yet

2 participants

@benlangfeld
Contributor

Fixes #712

@benlangfeld
Contributor

Of course, this now doesn't work on JRuby. I need to move the changes in Node#[] into the C extension.

@benlangfeld
Contributor

After doing bloody-battle with the C code, and a brief drop into Java, this is the best solution I have managed to come up with.

@yokolet Any thoughts?

@benlangfeld
Contributor

Can this be reviewed/merged please?

@benlangfeld
Contributor

Anything I can do?

@benlangfeld
Contributor

@flavorjones @jvshahid @yokolet: I'd really, really like to get this change in to 1.5.6. Can I get some feedback? I'd love to not have to move away from Nokogiri but this is becoming a deal-breaker for me.

@flavorjones
Member

When I run the test suite on this branch under jruby-1.6.7.2 I get the following error:

  1) Error:
test_set_prefixed_attributes(Nokogiri::XML::TestNodeAttributes):
NoMethodError: undefined method `href' for nil:NilClass
    ./test/xml/test_node_attributes.rb:37:in `test_set_prefixed_attributes'
    org/jruby/RubyKernel.java:2076:in `send'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:942:in `run'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:939:in `run'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:781:in `_run_suite'
    org/jruby/RubyArray.java:2331:in `collect'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:774:in `_run_suite'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:764:in `_run_suites'
    org/jruby/RubyArray.java:2331:in `collect'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:764:in `_run_suites'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:740:in `_run_anything'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:903:in `run_tests'
    org/jruby/RubyKernel.java:2076:in `send'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:890:in `_run'
    org/jruby/RubyArray.java:1615:in `each'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:889:in `_run'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:878:in `run'
    /home/miked/.rvm/gems/jruby-1.6.7.2/gems/minitest-2.2.2/lib/minitest/unit.rb:658:in `autorun'
    org/jruby/RubyProc.java:270:in `call'
    org/jruby/RubyProc.java:224:in `call'

1159 tests, 2787 assertions, 0 failures, 1 errors, 10 skips
rake aborted!
Command failed with status (1): [/home/miked/.rvm/rubies/jruby-1.6.7.2/bin/...]
@benlangfeld
Contributor

Unfortunately I'm no longer able to run the specs on JRuby, due to this issue with racc. Any ideas?

@benlangfeld
Contributor

Disregard that last comment, merging master in solved that issue. Working on fixing the spec failure.

@benlangfeld
Contributor

So, re-familiarising myself with the situation, that bug was documented by this failing test. Unfortunately, my knowledge of the Java code is not great. What I need to do is get hold of the namespace definition for a particular prefix on the node (or its ancestors) in order to set that when doing:

node['foo:bar'] = 'baz'

Can someone help me out with how I might get hold of that namespace definition?

@benlangfeld
Contributor

Can we get this merged so the failure is at least infront of everyone? This is broken in master anyway, just hidden. There should be a failing test for it until it can be fixed.

@flavorjones
Member

I've merged the failing tests onto master. I'd like to take a whack at fixing this.

@benlangfeld
Contributor

And what of the prefixed Node#[]?

@flavorjones
Member

@benlangfeld - Are you referring to the behavior described by your tests? If so, as I said above, I'm going to write the code to make the tests pass. If not, can you be more specific with your question?

@benlangfeld
Contributor

You only merged the commits from this pull request which describe the current failure on jruby, but not the earlier ones which normalise the getter behaviour between jruby and the c extension, which form the bulk of this pull request (a whole bunch of C code to match JRuby, whose behaviour I consider correct as outlined in the linked issue this pull request fixes).

@flavorjones
Member

Are we in agreement on these points:

  • You specifically asked to merge the test failures so that it was obvious that the behavior is wrong.
  • I cherry-picked the tests from this PR.
  • I did not merge your code changes from this PR.
  • There are currently failures on both MRI and JRuby, which accurately describe the desired behavior.

?

This pull request is messy. You have an unused private method implemented in C hanging around; you have a Ruby commit that is later reverted by your C implementation; and you're using regular expressions where they're not necessary.

I will make these tests pass by implementing a cleaner patchset. You are welcome to submit a new PR that addresses the above issues.

@benlangfeld
Contributor

Sure, we agree on those points.

As for the messy change-set:

  1. The commits show progress in prototyping this functionality and then moving it down to C. Can you point me to the section in the contrib docs which states pull requests should be squashed?
  2. I'd like to see your implementation that doesn't use a regex and does fewer operations on the string.
  3. I asked repeatedly for a code review and admitted that my C implementation was less than perfect. This is not code I'm used to, and I threw something together hoping for feedback on how it might be improved such that it would become acceptable to merge.

I don't mean to appear ungrateful for your attention here, but your last comments are rather harshly worded considering 3 months of repeated requests for comments. One does not spend half a day preparing a pull request, despite the fact that you might be able to do a better job in 10 minutes, hoping to be berated for doing a less than perfect job.

@flavorjones
Member

Ben,

I politely declined to merge the code, and you seemed to not understand why, so I explained. This should be interpreted as constructive feedback, and not "berating".

I don't mind implementing the fix. I'm not complaining, and I'm not attacking. I apologize that it took so long to get feedback; we all have day jobs and families here at Team Sparklemotion, and sometimes things fall through the cracks.

Your contribution on this issue has been extremely valuable, thank you again.

@benlangfeld
Contributor

Can we get an RC with this change?

@flavorjones
Member

Yes. I'll put something together today/tonight.

@flavorjones
Member

1.5.6.rc3 was released earlier this week.

@dc7kr dc7kr referenced this pull request in roo-rb/roo Mar 2, 2013
Merged

Fixes for ODS reading with nokogiri 1.5.6 #18

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