-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[ji] package (internal) refactoring #3754
Conversation
Since JavaPackage extends Module I do not see this change as one which could possibly break anything (knock on wood). I am interested in an enumerated list of package names which would be invalid before and after the change. At a minimum, having a list like this would be great for documentation. module JavaPackageModuleTemplate disappears from Java since it is private but you also remove the Ruby version as well. I wonder if anyone in the wild uses this (perhaps alias JavaPackage to this just in case?). I know you did not do this based on diff but there are many method calls which are prefixed with 'this.'. If you want to kill those that would be nice. You changed visibilty of method_missing and const_missing to private which I think is correct but I am just mentioning it in case someone knows why non-private would still be needed or could cause problems. All-in-all I have no issues with the PR itself my only question would be to challenge which behavior could have possibly changed which will generate an issue report. |
as noted in the desc it stayed the same (for now) except for : so really only
will look into some github usage if anyone has done something of a kind. if not I'll leave it dead as is.
NP - will do, thanks.
yeah I noticed that and fixed in a follow-up commit: 744bf4e ... so this should be the same as it was. |
with makeMetaClass - previous package module meta-class attaching ... should no longer be necessary
- `pkg.name` has been working since 1.7.22/9.0.1.0 (#2468) - handling :object_id as it is quite surprising to not have - can handle :throw/:catch since they're not valid package names - commented-out methods that would be good to have as well
and assert that `pkg.object_id` works as expected!
8ece95a
to
80966be
Compare
addressed the rest and squashed some of the commits for more clarity. UPDATE: so those are all copies of old JRuby's stdlib files - no usage found really. |
…bility ... its probably fine to remove - but just in case
… right ... also restored singleton method hooks for compatibility with previous versions
for the record - dealing with the introduced proxy class initialization regression: f1280b1 |
@kares can this get cherry-picked to 1.7 also? |
@enebo these (package) changes are not there -- thus makes no sense, or does it 😸 ? |
@kares oh...yeah I don't know why it is happening then. I figured it was the same issue. Weird that both branches have the same failing test. |
// cc @Lan5432 - sorry forgot, its been merged already but could be interesting to learn some JI internals |
Yeah, I will take a look into the commits and the discussion. Thanks anyways, I know the 9.1 release is an important task |
mostly some cleanup around
JavaPackageTemplate
which is replaced by a nativeJavaPackage
... allows for pieces to stay at one place - and avoid 'hacks' such as setting
@package_name
.compatibility should be fine, I see too (minor) "breaking" changes :
java.lang.class
will returnJava::JavaPackage
instead ofModule
(believe this is for the better)was thinking about exposing more methods (with the BlankSlateWrapper hack) on packages, but for now I hold this back (left them commented) :
singleton_class
sinceclass
andname
work as expected and its very low risk to collidethrow
,catch
as those are Java keywords packages couldn't use those namesSOME (FUTURE) IDEAS :
we might allow packages to behave like a Ruby object (with a
-Xji
switch) - no method filteringfor conflicting package names - method access wouldn't work but constant access still might :
e.g.
org.jruby.test.name.MyClass
🔴 (duename
) butorg::jruby::test::name::MyClass
🍏 but maybe its not such a good idea - let me know what are your thoughts on that. currently users need to resort toJava::OrgJrubyTestName::...
orjava_import 'org.jruby.test.name'
in such cases.numbers show performance is around the same if not better on some runs :
9.0.5.
9.1.0