Pathname special case on parse fixes #2110, #1821 #2111
Conversation
|
Code Climate has analyzed commit 88479cb and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 94.3% (0.0% change). View more on Code Climate. |
|
lol code climate you are dumb |
|
✅ Build nokogiri 1.0.784 completed (commit cff286467e by @doriantaylor) |
|
Don't worry about Code Climate, I marked it as WONTFIX. Looking into the other build failure now. |
|
The failing test is a flake, I kicked it and it should go green shortly. Looking at the proposed changes now. |
|
I'm kinda lukewarm on having three distinct sites where |
|
@doriantaylor I agree, and the reason I haven't moved yet on my promise in #1821 to write my own PR is because I intended to overhaul the parsing logic and try to consolidate it into fewer places; and I haven't had the time; and it's been a low priority because of the ease of the workaround. I'm cleaning up a couple of issues and will force-push to this branch in a bit along with an explanation of what I'm changing. Hope that's OK, and I'll keep your name on the commits. |
flavorjones
force-pushed
the
2110-pathname-special-case
branch
from
7f7eee8
to
0892a4a
Compare
|
|
||
| if string_or_io.is_a?(Pathname) | ||
| # resolve the Pathname to the file and open it as an IO object, see #2110 | ||
| string_or_io = string_or_io.expand_path.open |
There was a problem hiding this comment.
@tenderlove can I get your eyes on this? I'm pretty sure File objects autoclose when they are GCed (and I tested that), but figured it was worth asking explicitly.
There was a problem hiding this comment.
(My inclination was to throw expand_path in there because you can have legitimate stuff like Pathname('~/foo.xml') and the overhead is minimal.)
There was a problem hiding this comment.
oh btw putting the url assignment after the Pathname test was deliberate because Pathname does not respond to path but Pathname#open will produce a File which will respond to path.
There was a problem hiding this comment.
@doriantaylor I don't have any concerns about #expand_path, I just want to confirm with @tenderlove that the File object we're creating will get cleaned up and file descriptors will get closed during object finalization.
Your url comment makes sense -- what confused me was that after your initial patch set (which I've squashed), it was done one way in HTML::Document and the other way in XML::Document. I'll add some test coverage to make sure it's working the way we expect.
|
✅ Build nokogiri 1.0.785 completed (commit 23cc0d9fce by @flavorjones) |
- no need to resolve Pathname in Nokogiri.parse - added test coverage for XML::Document and HTML::Document#parse - moved resolution of Pathname to after `url` is set in HTML::Document#parse - open files with implict "r" because "b" assumes ASCII-8BIT encoding Part of sparklemotion#2110, sparklemotion#1821
flavorjones
force-pushed
the
2110-pathname-special-case
branch
from
0892a4a
to
4e3ed1f
Compare
- added test coverage for objects that respond to `#path` - added test coverage for `Pathname` specifically Related to sparklemotion#2110.
flavorjones
force-pushed
the
2110-pathname-special-case
branch
from
4e3ed1f
to
88479cb
Compare
|
✅ Build nokogiri 1.0.788 completed (commit 1987468dfa by @flavorjones) |
|
@doriantaylor Merged! Thanks for your contribution! |
|
Nice! Thanks for merging so promptly! |
What problem is this PR intended to solve?
The mangled behaviour of
Pathnameobjects; closing #2110 and #1821.Have you included adequate test coverage?
I hope so? The logic on
Nokogiri.parseis a little funky but it seemed a little funky to begin with.Does this change affect the C or the Java implementations?
From what I can tell this change operates above the underlying implementation.