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

Hybrid (native/ruby) pathname library #857

Merged
merged 3 commits into from Jul 6, 2013

Conversation

Projects
None yet
3 participants
@eregon
Copy link
Member

eregon commented Jul 5, 2013

This approach is quite similar to how MRI does it.
A good part of the library is still written in Ruby and it would take a considerable effort to translate everything (plus, it might not be very worthy). I think this should be merged in a first step.

The binding @path <=> path Java field is done by setting @path in #initialize, is there a better way?

There are a couple TODOs left, to which some light on it could help me.

Updated version of lib/ruby/1.9/pathname.rb is HEAD of the ruby_1_9_3 branch (only the require line changes from pathname.so to pathname_ext (is there a better naming, possible to differentiate pathname.{rb,jar}?
Should we add back the doc there? (in MRI, it was moved to pathname.c)

About the license, is it copied and adapted rightly?

Tests ($ bin/jruby test/externals/ruby1.9/pathname/test_pathname.rb) give:

263 tests, 520 assertions, 2 failures, 1 errors, 0 skips

The error is due to IO.open not being fully 1.9-compliant.
The first failure is ENOENT not raised with File.realdirpath (I will try to fix this).
The second failure is ENOENT being raised instead of ELOOP (symlink loop).

@headius what do you think?

.getVariableTableManager()
.getVariableAccessorsForRead()
.put("@path",
cPathname.getVariableTableManager().getVariableAccessorForWrite("path"));*/

This comment has been minimized.

Copy link
@eregon

eregon Jul 5, 2013

Author Member

Is there a good way to bind the java field with the instance variable?

This comment has been minimized.

Copy link
@headius

headius Jul 6, 2013

Member

Unfortunately not. Either use an instance var or a field, and generally only use the instance var if there's significant code in the wild that expects it to be there. The field will usually be faster.


public static RubyPathname newInstance(ThreadContext context, RubyClass klass, IRubyObject path) {
RubyPathname pathname = new RubyPathname(context.runtime, klass);
return (RubyPathname) pathname.initialize(context, path);

This comment has been minimized.

Copy link
@eregon

eregon Jul 5, 2013

Author Member

Is this the standard way to do it?

This comment has been minimized.

Copy link
@headius

headius Jul 6, 2013

Member

This all looks pretty standard, yes.

@JRubyMethod
public IRubyObject each_entry(ThreadContext context, Block block) {
if (block.isGiven()) {
// TODO: yield block while iterating

This comment has been minimized.

Copy link
@eregon

eregon Jul 5, 2013

Author Member

It's probably OK for now since even Dir.foreach gets an Array of all entries.

for (int i = 0; i < paths.size(); i++) {
RubyString path;
if (paths.get(i) instanceof String) // TODO
path = context.runtime.newString((String) paths.get(i));

This comment has been minimized.

Copy link
@eregon

eregon Jul 5, 2013

Author Member

I did not get how a Java String could be in a RubyArray, maybe I should just use sth else than get(i)?

This comment has been minimized.

Copy link
@headius

headius Jul 6, 2013

Member

The implementations of the Java Collection/List/Map interface always attempt to coerce to a "natural" Java object; in this case, that would produce a java.lang.String. Use one of the elt* functions, probably elt() or eltOk().

@headius

This comment has been minimized.

Copy link
Member

headius commented Jul 6, 2013

The PR looks like a good start, and I agree we should merge it. I'll see if I can make that happen, since it's not automatically mergable with recent source tree reorg.

@jrubyci jrubyci merged commit 10d22da into jruby:master Jul 6, 2013

1 check failed

default The Travis CI build failed
Details
@headius

This comment has been minimized.

Copy link
Member

headius commented Jul 6, 2013

Merged! Make sure you rebase to the new layout and read BUILDING.md for changes to the dev workflow. Looking good so far!

eregon added a commit that referenced this pull request Jul 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.