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

Remove redundant paragraph from systems.md #5

Merged
merged 1 commit into from Nov 29, 2018
Merged

Remove redundant paragraph from systems.md #5

merged 1 commit into from Nov 29, 2018

Conversation

AridTag
Copy link
Contributor

@AridTag AridTag commented Nov 28, 2018

This paragraph is already stated in the opening paragraph for System Types

This paragraph is already stated in the opening paragraph for `System Types`
@grofit
Copy link
Member

grofit commented Nov 28, 2018

Hey, thanks for the PR.

I am not sure if the paragraph is redundant though.

The paragraph you removed indicates to the user that they can mix and match the interfaces however they wish, which is useful to know.

The 2nd paragraph indicates that their loading order can be given priority and alters the running order and does not really touch on the mixing and matching of interfaces.

I am happy to change the docs if you would prefer it laid out differently, but I do not see any overlap/duplication of information between these 2 paragraphs.

Let me know what you think and we can discuss further.

@AridTag
Copy link
Contributor Author

AridTag commented Nov 28, 2018

This is a quote from the first paragraph under System Types

. You can also mix them up so you could have a single system implement ISetupSystem, ITeardown and IReactToEntitySystem which would run a setup method for each entity when it joins the group then react to the entity changes and process them on changes,

While this is the paragraph I removed

One other thing worth mentioning is that you can implement many of the interfaces in one class if you wish, so if you want an ISetupSystem and an ITeardownSystem implemented in the same class, you can do so and it will all just work, so implement as much or as little as you wish for your scenarios.

To me these 2 bits of text tell me the same thing, but if you'd rather leave it how it is that's fine

@grofit
Copy link
Member

grofit commented Nov 29, 2018

@AridTag So sorry, I just saw the change log and it implied changes to 2 of the lower sections so didnt realize the duplicate was up top, will approve it now and thanks again!

@grofit grofit merged commit 7ad144b into EcsRx:master Nov 29, 2018
@AridTag
Copy link
Contributor Author

AridTag commented Nov 29, 2018

No worries I thought that might be the case. Not sure why the diff shows up like that

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

Successfully merging this pull request may close these issues.

None yet

2 participants