Skip to content

feat(extract-subsystems): extract Context, @In, InjectionHelper and @Share to Subsystem #4930

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

feat(extract-subsystems): extract Context, @In, InjectionHelper and @Share to Subsystem #4930

wants to merge 5 commits into from

Conversation

@DarkWeird
Copy link
Contributor

@DarkWeird DarkWeird commented Oct 25, 2021

Contains

Part of #4304.

Provide possible to Injectable classes(@Share, @In and Context usage) with another subsystems.

Should be replaced with gestalt v8 (di)

How to test

1.Run your favorite game (hint: Terasology)
2. Check it don't fail whit null pointer exception at @In field.

Outstanding before merging

@github-actions github-actions bot added the Type: Improvement Request for or addition/enhancement of a feature label Oct 25, 2021
@DarkWeird DarkWeird mentioned this pull request Oct 25, 2021
Open
@DarkWeird
Copy link
Contributor Author

DarkWeird commented Nov 10, 2021

Context Subsystem extraction

keturn
keturn suggested changes Nov 15, 2021
View reviewed changes
Copy link
Member

@keturn keturn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI did not run any org.terasology.engine.registry tests: https://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/PR-4930/lastCompletedBuild/testReport/

subsystems/Context/src/test/java/org/terasology/engine/registry/CoreRegistryTest.java
public void testContextChange() {
CoreRegistry.setContext(new ContextImplementation());
assertNotEquals(CoreRegistry.get(Context.class), context);
Assertions.assertNotEquals(CoreRegistry.get(Context.class), context);
Copy link
Member

@keturn keturn Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentional? (Adding the Assertions. qualifier back in, instead of a static import of the assertNotEquals method.)

Copy link
Contributor Author

@DarkWeird DarkWeird Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just personal prefer to avoid static imports

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Nov 16, 2021 •
edited
Loading

@keturn

CI did not run any org.terasology.engine.registry tests: https://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/PR-4930/lastCompletedBuild/testReport/

Nice catch!

Our pipeline ignores test task.
test-related tasks(in run order):

  1. gradle unitTest
  2. gradle check -x test (-x test - excludes test task)
  3. gradle integrationTest

This affects all :subsystems:* :(

@DarkWeird DarkWeird mentioned this pull request Nov 23, 2021
Open
@DarkWeird DarkWeird requested a review from keturn November 29, 2021 11:47
@DarkWeird
Copy link
Contributor Author

DarkWeird commented Nov 29, 2021

about test task - not this PR issue

@jdrueckert
Copy link
Member

jdrueckert commented Oct 23, 2022

@DarkWeird In today's contributor meeting the question came up, whether it makes sense to invest into this when the "proper" way to do this seems to be gestalt v8...?
fyi @keturn

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Oct 24, 2022

@DarkWeird In today's contributor meeting the question came up, whether it makes sense to invest into this when the "proper" way to do this seems to be gestalt v8...?
yeah :D gestalt v8 is "proper" way

see description:

Should be replaced with gestalt v8 (di)

this pr is trying to split injections before gestalt v8 usage

@jdrueckert
Copy link
Member

jdrueckert commented Oct 26, 2022

@DarkWeird Hehe okay, let me rephrase my question: What's the benefit ofextracting these things into a subsystem before using gestalt v8?

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Oct 27, 2022

@DarkWeird Hehe okay, let me rephrase my question: What's the benefit ofextracting these things into a subsystem before using gestalt v8?

Path to extract another things (to subsystems) before then gestalt v8 will be integrated.
This submodule easy to remove when gestaltv8 di will be integrated

@jdrueckert
Copy link
Member

jdrueckert commented Oct 28, 2022

From my pov, if this makes it easier to get rid of it after migrating to gestalt v8, I'm fine with reviewing and merging it (provided you resolve the conflicts first 😅 )
However, IIRC from our last meeting, keturn had some concerns about the required effort to make this a subsystem first. @keturn can you comment on the details of your concerns some more before we move on with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skaldarnar skaldarnar Awaiting requested review from skaldarnar

@jdrueckert jdrueckert Awaiting requested review from jdrueckert

@keturn keturn Awaiting requested review from keturn

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Type: Improvement Request for or addition/enhancement of a feature

Projects

Terasology Stabilization
Status: 🏗 In progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@DarkWeird @jdrueckert @keturn @pollend