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

All objects in EOorg package must be package private #3059

Closed
maxonfjvipon opened this issue Apr 6, 2024 · 15 comments
Closed

All objects in EOorg package must be package private #3059

maxonfjvipon opened this issue Apr 6, 2024 · 15 comments
Assignees

Comments

@maxonfjvipon
Copy link
Member

For some reason all atoms and generated from EO java object has public in front of class and constructor.
There's no need for that. All of them should be package private

Copy link

github-actions bot commented Apr 6, 2024

@maxonfjvipon thanks for the report, but here some unclear moments:

  • The bug report lacks specific details about the problem. It mentions that all atoms and generated from EO java object has public in front of class and constructor but does not provide any examples or instances where this issue is occurring. To fix this, the author should provide specific instances where this issue is found.
  • The bug report does not provide any steps to reproduce the issue. This makes it difficult for the developers to understand and fix the issue. The author should provide a step-by-step guide on how to reproduce the problem.
  • The bug report does not describe the expected and actual results. The author mentions what should be the case, but does not mention what is currently happening. The author should clearly state the expected result and what actually happens.

Please fix the bug report in order it to get resolved faster.
Analyzed with gpt-4

@levBagryansky
Copy link
Member

@maxonfjvipon I can do it

@levBagryansky
Copy link
Member

@maxonfjvipon Looks like we cannot do it since EOorg/ objects are used from PhPackage via reflection: https://github.com/objectionary/eo/blob/master/eo-runtime/src/main/java/org/eolang/PhPackage.java#L155
So I get errors like
"Error at "EOorg.EOeolang.EOfloat$EOeq#φ" attribute; caused by Can't find Java object/package 'EOorg.EOeolang.EOif' in EO package 'org.eolang'; caused by IllegalAccessException: class org.eolang.PhPackage cannot access a member of class EOorg.EOeolang.EOif with modifiers "public"")

@maxonfjvipon
Copy link
Member Author

@levBagryansky I think there should be a way to get access to package private constructor via reflection

@levBagryansky
Copy link
Member

@maxonfjvipon It's not clean to use reflection to circumvent our own rules. I try to solve it in java semantic

@maxonfjvipon
Copy link
Member Author

@yegor256 it seems to resolve it we should do something like:

Constructor<Phi> constructor = constructor = Class.forName(target).getConstructor(Phi.class)
constructor.setAccessible(true);
Phi phi = constructor.newInstance(Phi.Ф);

WDYT?

@yegor256
Copy link
Member

@maxonfjvipon @levBagryansky I suggest we close this issue. Reflection is not a good idea in this case.

@levBagryansky
Copy link
Member

@yegor256 @maxonfjvipon Now PhPackage depends on EOorg/ classes. I think we can create a public class
in EOorg/ and interface for it in org/eolang package in order to reverse this dependency.

@maxonfjvipon
Copy link
Member Author

@levBagryansky let's try. If there's no luck, then we should just make sure all classes in EOorg/ package has the same access level.

@levBagryansky
Copy link
Member

@maxonfjvipon or we can create a public class in EOorg/ that will provide ctors to other classes so PhPackage will use it.

@maxonfjvipon
Copy link
Member Author

@levBagryansky looks like some kind of portal from org.eolang to EOorg.EOeolang package. I don't think it's a good idea.
@yegor256 we already use reflection here because there's no other way to get object by name.
So there are two options:

  1. We extend this reflection with constructor.setAccessible(true);. It allows us to make all classes in EOorg/ package private
  2. We don't touch the code. Then I think we should make all classes in EOorg/ public just for consistency.

What's your desicion?

@levBagryansky
Copy link
Member

@maxonfjvipon wait for my pr

@yegor256
Copy link
Member

@maxonfjvipon @levBagryansky setAccessible(true) is pure evil. I vote for the second option.

@levBagryansky
Copy link
Member

@yegor256 @maxonfjvipon I faced the problem that inner packages like EOio will not be accessible for my EOorg.EOeolang.PhiFactory so I just will do them all public

@maxonfjvipon
Copy link
Member Author

Done in #3103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants