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: custom context #29

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

whilefoo
Copy link

@whilefoo whilefoo commented Oct 23, 2023

This implements custom context type which includes event context and bot config.
@pavlovcik

@sweep-ai
Copy link

sweep-ai bot commented Oct 23, 2023

Apply Sweep Rules to your PR?

  • Apply: Leftover TODOs in the code should be handled.
  • Apply: All new business logic should have corresponding unit tests in the tests/ directory.
  • Apply: Any clearly inefficient or repeated code should be optimized or refactored.

Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure this is sufficient to solve the business problem? It doesn't look like it.

Copy link
Author

Choose a reason for hiding this comment

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

Why not?

Copy link
Owner

Choose a reason for hiding this comment

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

I expected it to be a lot more involved with some type of array or object with getters and setters.

Copy link
Author

Choose a reason for hiding this comment

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

I can change it a class with helper functions like getOctokit, getPayload, getEvent, getConfig but other than that I don't see what else could be added

Copy link
Owner

Choose a reason for hiding this comment

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

Its fine. Can you fix the build errors

Copy link
Author

Choose a reason for hiding this comment

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

two build errors in processors.ts are from your code so I'm not sure why it's not working and one error is from wallet.test.ts because now it needs a mocked context but I don't think that test is even needed, should I just remove it?

Copy link
Owner

@0x4007 0x4007 Oct 24, 2023

Choose a reason for hiding this comment

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

I think we should keep all the tests. It seems like a waste to write tests and then delete instead of fixing them when new code is introduced. Please address those and I'll merge it in.

Also please feel free to refer to my latest for fixes etc which currently is at https://github.com/pavlovcik/ubiquibot/tree/refactor/permit-generation

Copy link
Author

Choose a reason for hiding this comment

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

I just realized that adapters will have to be moved into context aswell, because some tables like logs and wallet use the context, and having adapters in runtime will mean they will override on each request.

There are two choices:

  1. change adapters' functions to accept additional parameter context so they can stay as global runtime
  2. move adapters to context

Copy link
Author

Choose a reason for hiding this comment

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

@pavlovcik what do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Hard to say cause I dont think I understand the tradeoff well enough.

Copy link
Owner

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

There's some build errors and questions.

@0x4007 0x4007 merged commit 51d069f into 0x4007:refactor/general Oct 30, 2023
3 of 5 checks passed
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