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

Add log metadata plugin #865

Merged
merged 15 commits into from
Dec 12, 2020
Merged

Add log metadata plugin #865

merged 15 commits into from
Dec 12, 2020

Conversation

tomasvidhall
Copy link
Contributor

@tomasvidhall tomasvidhall commented Dec 11, 2020

Tries to solve the second task in #227

2. Custom payload
Allow insertion of custom messages into each log event.

I read up on how it used to work in #267 and tried to recreate something similar to that.

Added a Log metadata plugin which can be reached using ctx.log.setMetadata() and sets the payload as metadata on the added LogEntry.

Screenshot 2020-12-11 at 10 22 18

I a bit unsure about the naming of metadata, depends what you feel the terminology should be. I tried to find any mismatches and the closest I got was that there is something called action metadata for the bots, but it shouldn't collide from what I could see.

I don't usually write Typescript or Svelte, so if I missed something please point it out and I'll fix it. 🚀

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).

@tomasvidhall
Copy link
Contributor Author

Forgot to add reviewers, labels etc and now I can't edit them, please help! 🤦

@tomasvidhall tomasvidhall mentioned this pull request Dec 11, 2020
6 tasks
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Hi @tomasvidhall thanks for putting this together — looks good!

There are quite a few comments in this review, so apologies if it’s a little confusing. Please ask if anything is unclear.

Basically what I’m suggesting is

  1. Move metadata out of action.payload and into the top-level log entry itself.

  2. Some minor tweaks to the plugin API/data.

I think the rest is mainly Typescript advice.

src/client/debug/log/LogEvent.svelte Outdated Show resolved Hide resolved
src/plugins/plugin-log-metadata.ts Outdated Show resolved Hide resolved
src/plugins/plugin-log-metadata.ts Outdated Show resolved Hide resolved
src/plugins/plugin-log-metadata.ts Outdated Show resolved Hide resolved
src/plugins/plugin-log-metadata.ts Outdated Show resolved Hide resolved
src/plugins/plugin-log-metadata.ts Outdated Show resolved Hide resolved
src/plugins/plugin-log-metadata.ts Outdated Show resolved Hide resolved
packages/plugins.ts Outdated Show resolved Hide resolved
src/core/reducer.ts Outdated Show resolved Hide resolved
@tomasvidhall
Copy link
Contributor Author

Thanks for the great review @delucis. I've updated the PR with the suggested changes. Feel free to give me more feedback.

@delucis
Copy link
Member

delucis commented Dec 12, 2020

Changes look good @tomasvidhall! Will run a few local tests when I’m back at my computer, but I think this looks ready to merge.

@delucis delucis merged commit 3918cc9 into boardgameio:master Dec 12, 2020
@delucis delucis mentioned this pull request Dec 13, 2020
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.

2 participants