[WIP] Pushing towards using Pathname consistently. Closes #216

[bartkamphorst]

Related to #216. Note that while the gollum-lib specs pass, the gollum specs do not. Including these changes therefore requires changing gollum's code as well.

[dometto]

Good stuff! Is there an easy way to detect where else this needs to happen?

[bartkamphorst]

Not really, I follow the request chain, and keep an eye out for ::File.dirname calls etc (these accept Pathname objects but return strings). There are many changes still to make, as there are many places where there are checks for '/' or '.' etc.

[dometto]

Tests are failing on jruby because Page#path now returns a Pathname object. On MRI this is automatically interpreted as a string, but apparently not so on java:

TypeError: cannot convert instance of class org.jruby.ext.pathname.RubyPathname to class java.lang.String

The error is raised when Page#path is handed to the adapter, in two places (so far):

git_layer_rjgit.rb:384:in `diff'
git_layer_rjgit.rb:212:in `log'

Three basic ways of fixing this:

• Explicitly convert Page#path to a string before handing it to the adapter (at least in Wiki#full_reverse_diff_for and Page#last_version)
• Explicitly convert to string inside the adapter (in the above mentioned methods)
• Monkeypatch or subclass Pathname so that it allows conversion to java.lang.String

[bartkamphorst]

Explicitly convert to string inside the adapter (in the above mentioned methods)

This option has my vote. I'd like to use Pathnames throughout the project, and explicitly call .to_s where external methods force us to.

[dometto]

I resolved the merge conflicts which arose because of #249.

[bartkamphorst]

@dometto Did this end up in gollum-lib-5.x some way or other?

[dometto]

Argh. No, looks like I accidentally deleted the branch on which this PR was based. I think we can restore it though.

[bartkamphorst]

👍

[dometto]

Restored and also resolved a new merge conflict. You may have to recheck the page.rb file for new code that may be improved by using Pathname though. I see I've used ::File.basename and such here and there when refactoring the Page class.

Sorry for the messup!

[bartkamphorst]

No worries, just glad my work still has a chance of being included in the next big release. ;)

[dometto]

Just solved another merge conflict. Tests still fail, but that all seems to be due to some mistaken calls to Pathname#include? (mostly in the tests themselves), which should be easy to solve.

Related to #216. Note that while the gollum-lib specs pass, the gollum specs do not. Including these changes therefore requires changing gollum's code as well.

What needs to happen to gollum before this can be merged?

[bartkamphorst]

Just solved another merge conflict. Tests still fail, but that all seems to be due to some mistaken calls to Pathname#include? (mostly in the tests themselves), which should be easy to solve.

Very cool. 👍

What needs to happen to gollum before this can be merged?

I don't remember to be honest. We would just have to hook up this gollum-lib code to gollum and see.

[dometto]

Do we still want to get this into 5.x?

[bartkamphorst]

No, I think we can release without. I'm keeping it open though for future reference.