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

Prep for GameLog stuff: Introduce ContextEnhancer #261

Merged
merged 12 commits into from
Aug 24, 2018
Merged

Prep for GameLog stuff: Introduce ContextEnhancer #261

merged 12 commits into from
Aug 24, 2018

Conversation

Stefan-Hanke
Copy link
Contributor

@Stefan-Hanke Stefan-Hanke commented Aug 20, 2018

This PR is written as prep work for #227, where we want to attach a log object to the context, allowing a game to write custom logging information. It introduces a class called ContextEnhancer that ties all APIs together that can be attached and detached from a context - currently, these are random and events. This results in

  • less code duplication, and
  • easier attaching of new APIs.

Albeit I failed to also fixup the code creating the initial state, I've not exactly pinpointed why using ctxApi.update() does not work. The flow of the various state and context objects gets unwieldy, too. And perhaps there are other places that could use that class also.

Let me know what you think.

Checklist

  • Use a separate branch in your local repo (not master).
  • Test coverage is 100% (or you have a story for why it's ok).

@coveralls
Copy link

coveralls commented Aug 20, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7f31d59 on Stefan-Hanke:allow_payload_in_move into 3999db3 on google:master.

update(ctx) {
return { ...ctx, _random: this.state };
update(state) {
var newCtx = { ...state.ctx, _random: this.state };
Copy link
Member

Choose a reason for hiding this comment

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

This can be written more concisely as:

const ctx = { ...state.ctx, _random: this.state };
return { ...state, ctx };

initial.ctx = Random.detach(initial.ctx);
initial.ctx = Events.detach(initial.ctx);
initial.ctx = apiCtx.random.update(initial).ctx;
initial.ctx = apiCtx.detachFromContext(initial.ctx);
Copy link
Member

Choose a reason for hiding this comment

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

  state = apiCtx.update(state);
  initial.ctx = state.ctx;
  initial.ctx = apiCtx.detachFromContext(initial.ctx);

should work here.

A more interesting observation is given that update is always followed by a detachFromContext, shall we combine them into one call detach that accepts state and returns state?

@nicolodavis nicolodavis merged commit 510977f into boardgameio:master Aug 24, 2018
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

Successfully merging this pull request may close these issues.

3 participants