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

feat: add check for facet provider requirements #5073

Merged
merged 17 commits into from
Dec 17, 2022

Conversation

MrGizmo123
Copy link
Contributor

@MrGizmo123 MrGizmo123 commented Sep 3, 2022

Contains

Fixes #4924

Todo

  • Test that CoreGameplay world gen still works as expected
  • Test that focus gameplays' world gen still works as expected (JS, MR, LaS)
  • Test that logic correctly fails any attempt to add a facet provider with unsatisfied requirements
  • Fix failing tests
  • Write test case for new logic

@jdrueckert jdrueckert added this to the 5.4.0 milestone Sep 4, 2022
engiValk
engiValk previously approved these changes Sep 18, 2022
Copy link
Contributor

@engiValk engiValk left a comment

Choose a reason for hiding this comment

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

Seems fine to merge.

@jdrueckert
Copy link
Member

@MrGizmo123 I pushed a PR slightly changing the error log and adding throwing an IllegalStateException.
Thus, world generation fails and developers get direct and clear feedback when trying to test their world gen provider.
This can be tested, for instance, by adding FloraFacet.class to the required facets in CoreWorlds:BiomeProvider and trying to start a CoreGameplay world.

- Facet1Provider requires Facet2Provider but was added before it so far
@jdrueckert
Copy link
Member

Seems like the tests failed because the Facet1Provider in the test cases in WorldBuilderTest requires Facet2Provider but was added before it so far. This should be fixed now.

jdrueckert
jdrueckert previously approved these changes Oct 17, 2022
@jdrueckert
Copy link
Member

I added a test case that tests that if facet providers are added in an incorrect order, an exception is thrown.

Copy link
Member

@jdrueckert jdrueckert left a comment

Choose a reason for hiding this comment

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

It's been over 2 months now waiting for a review and while I would've loved to get one from @keturn who originally requested this feature, I believe it's more important to finally get this improvement in. We can always still revert or modify things afterwards in case we overlooked or misunderstood something about the feature request.

@jdrueckert jdrueckert changed the title Add check for facet provider requirements feat: add check for facet provider requirements Dec 17, 2022
@jdrueckert jdrueckert merged commit 2666812 into MovingBlocks:develop Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

FacetProvider requirements should be verified
3 participants