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

Posting capabilities #35

Open
wants to merge 12 commits into
base: development
Choose a base branch
from

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Aug 30, 2024

Resolves #44
Related to https://github.com/ubiquibot/kernel-logger/pull/2

For now only one storage integration is needed but once others are needed/ready the param can be made into a dynamic type which helps type the correct inputs needed for that storage location

With the configurability aspect of which levels to store the use of fatal should no longer be avoided.

@whilefoo
Copy link

whilefoo commented Sep 4, 2024

do you think it would be a good idea to make LogReturn extend Error because I remember there was an issue when throwing errors so we had to wrap in Error, for example: new Error(context.logger.error("test").logMessage.raw)

@Keyrxng
Copy link
Member Author

Keyrxng commented Sep 4, 2024

do you think it would be a good idea to make LogReturn extend Error because I remember there was an issue when throwing errors so we had to wrap in Error, for example: new Error(context.logger.error("test").logMessage.raw)

Yeah it practically does so already although metadata contains the Error, stack etc so should we just move those into log.error log.stack vs log.metadata.error?

@whilefoo
Copy link

whilefoo commented Sep 5, 2024

Yeah it practically does so already although metadata contains the Error, stack etc so should we just move those into log.error log.stack vs log.metadata.error?

Yes I think so

package.json Outdated Show resolved Hide resolved
@Keyrxng
Copy link
Member Author

Keyrxng commented Sep 7, 2024

Okay thinking about this more:

  • Error requires a call to super() and the problem previously was that tests were not detecting when we would throw LogReturn. Isn't now the reverse true that for every LogReturn that we create we are also instantiating a new Error class which is going to be caught by test suites in places where we likely do not intend for a trad error to be thrown.
  • Error optionally takes a msg and a cause which would now be available globally along with the stack etc.
  • Our Metadata already turns objects into errors and handles pretty formatting and we stringify the metadata as opposed to the global which would also include logMessage.raw/type...

I'm not sure that it is such a good idea to have LogReturn extend Error now. Any further thoughts on it?

@whilefoo
Copy link

whilefoo commented Sep 8, 2024

  • Error requires a call to super() and the problem previously was that tests were not detecting when we would throw LogReturn. Isn't now the reverse true that for every LogReturn that we create we are also instantiating a new Error class which is going to be caught by test suites in places where we likely do not intend for a trad error to be thrown.

I'm not sure I quite understand what's the problem with creating an Error class

  • Error optionally takes a msg and a cause which would now be available globally along with the stack etc.

as I understand the clause can either be another Error/LogReturn for example if you are rethrowing an error or it can be an object like our Metadata

  • Our Metadata already turns objects into errors and handles pretty formatting and we stringify the metadata as opposed to the global which would also include logMessage.raw/type...

We can still add our own properties for formatting and other stuff. I just like to use standardised types but it's your call if you think it's not feasible or doesn't add enough value

@Keyrxng
Copy link
Member Author

Keyrxng commented Sep 8, 2024

I'm not sure I quite understand what's the problem with creating an Error class

I gathered from our original problem that you mentioned that the issue was because we were not throwing an Error class error and so test suites were not detecting it correctly. I just thought that if we are creating a new Error class with every LogReturn tests would pick that up as unhandled errors but there is only one way to find out, it was just a thought.

as I understand the clause can either be another Error/LogReturn for example if you are rethrowing an error or it can be an object like our Metadata
We can still add our own properties for formatting and other stuff. I just like to use standardised types

I think it's a good idea as it makes error handling less verbose with conditionals etc but if it does cause issues with test suites I think it might be more hassle than it's worth

@whilefoo
Copy link

whilefoo commented Sep 8, 2024

I just thought that if we are creating a new Error class with every LogReturn tests would pick that up as unhandled errors but there is only one way to find out, it was just a thought.

Hmm I'm not sure but I don't see why the test suite would regard it as unhandled error - we can test to be sure

src/logs.ts Outdated Show resolved Hide resolved
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.

add supabase posting
4 participants