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

Exceptions should be passed as a class, and not an instance in order to simplify error stack trace #22

Open
CMCDragonkai opened this issue Sep 1, 2022 · 6 comments
Labels
enhancement New feature or request r&d:polykey:supporting activity Supporting core activity

Comments

@CMCDragonkai
Copy link
Member

Is your feature request related to a problem? Please describe.

The error stack trace is created when the error instance is constructed, not where it is thrown.

Atm, we fix this by overriding the error instance's stack trace at thrown time.

Describe the solution you'd like

If we take a class instead, we can just instantiate the class and then expect that the error instance is constructed. Even more general would be a function returning an error instance...

This will make the stack trace start from where it is thrown. But we may still need to "reset" it according to the decorated function, as that will give us a cleaner trace... but it assumes we don't need to know exactly where it is thrown. It's a bit more "magical".

I'm already trialling this out in the timed decorator and other context decorators.

Additional context

@CMCDragonkai CMCDragonkai added the enhancement New feature or request label Sep 1, 2022
@CMCDragonkai
Copy link
Member Author

This would be a breaking change, and would require fixes to all other downstream repos to change to the class. A large find & replace.

@CMCDragonkai
Copy link
Member Author

In the timed usage (MatrixAI/Polykey#297 (comment)), I've found more information is more useful. So I think in the case here, we may not really need to reset the stack trace at all once we have switched to using error class constructors instead of error instances.

It would give us a useful stack trace during debugging.

@CMCDragonkai
Copy link
Member Author

This will be scheduled for version 2 since it's a breaking change.

@CMCDragonkai
Copy link
Member Author

That means the resetStackTrace utility function can just be completely removed. And we just get the full information about the stack trace where it is thrown.

@CMCDragonkai CMCDragonkai self-assigned this Jul 10, 2023
@CMCDragonkai CMCDragonkai added the r&d:polykey:supporting activity Supporting core activity label Jul 10, 2023
@CMCDragonkai
Copy link
Member Author

This means for example in the function CreateDestroyStartStop, it currently takes the errorRunning and errorDestroyed as full Error instances.

Instead it should take Class<Error>, so it can be constructed in the appropriate place.

Same for the ready functions, it should just take Class<Error> as well so it can be properly constructed.

CMCDragonkai added a commit that referenced this issue Sep 3, 2023
…p and destroy

* take note that `CreateDestroyStartStop` takes an event dictionary,
this can be replaced in the future with an entire arg dictionary, this
should be done along with #22 and ESM migration as it is a breaking
change
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Sep 3, 2023

See #32. It has to be done first, before this issue can be resolved. Then a mass ESM migration can occur at the same time as doing the this issue's breaking changes.

The CreateDestroyStartStop should just take the full args as dictionary rather than taking errorRunning, then errorDestroyed. It should just take { errorRunning, errorDestroyed, eventStart, ... }.

Then breaking changes mean:

  1. ESM migration
  2. Supply classes of errors instead of supplying error instances
  3. Specify arg dictionary when using @CreateDestroyStartStop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request r&d:polykey:supporting activity Supporting core activity
Development

No branches or pull requests

1 participant