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

Refactor LoadService to make use of resources #1750

Merged
merged 4 commits into from Jul 9, 2014

Conversation

Projects
None yet
2 participants
@ratnikov
Copy link
Contributor

ratnikov commented Jun 16, 2014

This contains a very heavy refactor of require/load logic. The new version resides in a new LibrarySearcher class, whereas the old methods are deprecated and are not being invoked anymore.

There are four main paths for loading a file:

  1. builtin library
  2. Via a FileResource
  3. As a ServiceLibrary (when requiring 'foo' and java object FooService is being loaded)
  4. Via a classpath.

1-3 are now being handled by LibrarySearcher. 4) still resides as a backdrop in LoadService with intent to being deprecated as well, once a ClasspathResource implementation of FileResource exists.

@enebo

This comment has been minimized.

FoundLibrary libray = findBuiltinLibrary(...);

since the fist null check is always true

This comment has been minimized.

Copy link
Owner Author

ratnikov replied Jun 26, 2014

I did that mostly to be able to move the ordering around with easy. I'd imagine the compiler optimizing it out, but I'll fix it if you like.

This comment has been minimized.

Copy link

enebo replied Jun 26, 2014

yeah when I read it my mind realizes it is unneeded so I think I would eventually change it anyways :)

@enebo

This comment has been minimized.

All your code is 2-space indented but our project is all 4-space

This comment has been minimized.

Copy link
Owner Author

ratnikov replied Jul 13, 2014

I directly copied it from LoadService, where it used to be finName. =/

@enebo

This comment has been minimized.

finName is findName?

@enebo

This comment has been minimized.

Unrelated to your patch I think but for some reason I think we failed on server drive UNC things on windows like my_server:\foo.txt. This regexp makes me wonder if we do in fact load them since it handles more than drive letters.

Actually there is a question as to whether this is a valid regexp on non-windows machines? Did we also do this check without a platform guard?

This comment has been minimized.

Copy link
Owner Author

ratnikov replied Jul 13, 2014

Will replace with new File(...).isAbsolute in a separate PR.

@enebo

This comment has been minimized.

Not something for this PR but I wonder if this could be 3 types instead of one: JarResourceLibrary, ScriptResourceLibrary, etc..?

enebo added a commit that referenced this pull request Jul 9, 2014

Merge pull request #1750 from ratnikov/refactor-loadservice
Refactor LoadService to make use of resources

@enebo enebo merged commit d3459fe into jruby:jruby-1_7 Jul 9, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@enebo enebo added this to the JRuby 1.7.14 milestone Jul 9, 2014

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jul 9, 2014

My comments will be addressed by @ratnikov in future PR(s)

@enebo enebo modified the milestones: JRuby 1.7.14, JRuby 1.7.15 Aug 27, 2014

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.