Skip to content

Conversation

@stackmystack
Copy link
Collaborator

@stackmystack stackmystack commented Dec 5, 2024

We don't want our exceptions to go unnoticed or handled by default rescue.

Consumers need to handle them explicitly.

Copy link
Collaborator

@DerekStride DerekStride left a comment

Choose a reason for hiding this comment

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

This would be surprising to me as a user of the gem. Why don't we want them to be handled by default rescue blocks?

@stackmystack
Copy link
Collaborator Author

stackmystack commented Dec 5, 2024

Thanks for replying, I was hoping you'd see it today :)

This would be surprising to me as a user of the gem. Why don't we want them to be handled by default rescue blocks?

Take LoadError for instance. This is not a StandardError. If you look at LanguageLoadError, ParserNotFoundError, and ParserVersionError, at least, these are equivalent to a LoadError. You should absolutely handle them by name. In some sense, they are "unrecoverable" errors.

I would argue that SymbolNotFoundError and QueryCreationError are also similar in nature, in the sense that if you get them, then you're doing something deeply wrong … I will not defend this argument for those 2 errors with my life, but the first 3 are obviously very serious errors IMO. Maybe the strongest argument I can advance for these 2 to be this way is the fact that they inherit TreeSitterError, and treating them differently is a PITA.

I understand that the standard practice is to inherit StandardError, but maybe that applies to applications more than anything. For libraries, that makes me a bit uncomfortable.

WDYT?

Copy link
Collaborator

@DerekStride DerekStride left a comment

Choose a reason for hiding this comment

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

I would argue that SymbolNotFoundError and QueryCreationError are also similar in nature, in the sense that if you get them, then you're doing something deeply wrong

Hmm, the others make more sense now that you've laid it out. I'm still on the fence about these ones but also don't have strong feelings.

News.md Outdated
Comment on lines 5 to 9
# v1.9.2 (05-12-2024)

- Make `TreeSitter::TreeSitterError < Exception` instead of `StandardErorr`;
we don't want them to be handled by default `rescue`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This could arguably be a breaking change. I think we should at least bump to v1.10.0 or even v2.0.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with that. Let's bump to 1.10.0. I prefer bumping major when tree-sitter breaks some APIs, but I'm not religious about that. You prefer 2.0.0?

Copy link
Collaborator

Choose a reason for hiding this comment

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

1.10.0 is good with me 👍

@stackmystack stackmystack merged commit 4961538 into Faveod:master Dec 10, 2024
18 of 19 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