Skip to content

feat: add ScanErrorCode#52

Open
reneleonhardt wants to merge 2 commits intosaphyr-rs:masterfrom
reneleonhardt:feat/add-code-to-scanerror
Open

feat: add ScanErrorCode#52
reneleonhardt wants to merge 2 commits intosaphyr-rs:masterfrom
reneleonhardt:feat/add-code-to-scanerror

Conversation

@reneleonhardt
Copy link

Closes #29

Copy link
Contributor

@Ethiraric Ethiraric left a comment

Choose a reason for hiding this comment

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

Most of the changes I asked are just related to making the enum Copy.

I trust you on the naming. I'll try and keep in mind to make a pass on it before the next release, but I don't want to block this PR that long.

I wonder however how relevant it is to be this specific in errors. I probably would have kept the code limited to a list akin to

SyntaxError (maybe IndentError?)
DuplicateDirective
DirectiveError
EncodingError
UnexpectedEoF
UndeclaredAnchor

I'll ask @davvid and @eirnym for their opinions.


if string.is_empty() {
return Err(ScanError::new_str(start_mark, "while scanning an anchor or alias, did not find expected alphabetic or numeric character"));
return Err(ScanError::new_str(start_mark, "while scanning an anchor or alias, did not find expected alphabetic or numeric character", ScanErrorCode::AlnumCharacterExpected));
Copy link
Contributor

Choose a reason for hiding this comment

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

This might need formatting. Have you run cargo fmt?

Copy link
Author

Choose a reason for hiding this comment

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

If I change the import order cargo fmtcorrects it.
But something like long lines stays unchanged, at least for me.
I don't know why, there's no rustfmt.toml or .editorconfig.

@eirnym
Copy link

eirnym commented Jun 26, 2025

I working on a multilingual app and one of the native languages I'd like to have at start will be Polish.

If we keep list too broad and unsorted, it could be a mess, which I suppose @Ethiraric try to solve by proposing less errors. On the other hand, if we keep it too small, a user can't do much from "syntax error" translated on the target language.

To keep both of worlds I propose to split by categories proposed by @Ethiraric, but keep them rich by providing sub-codes to keep errors meaningful.

And all keep these enums non-exhaustive to add more in the future.

What do you @Ethiraric and @davvid think?

@Ethiraric
Copy link
Contributor

Oh, my bad. I mistakenly placed myself from the point of view of saphyr rather than saphyr-parser.

It would make sense to have categories still. I thought there was some ErrorCategory in std but I can't seem to find it now.
In light of that, I think we should have a pub enum Category (with the aforementioned field, IndentationError included) and an impl From<ScanErrorCode> for Category (just a giant match).
I'm also in favour of the non-exhaustive tag.

@eirnym
Copy link

eirnym commented Jun 26, 2025

What would impl From<ScanErrorCode> for Category do?

I thought on something simpler like following:

enum ScanErrorCode {
	InvalidUTF8(InvalidUTF8Code),
// ...
}

enum InvalidUTF8Code {
	IncorrectLeadingUTF8Byte,
// ...
}

@Ethiraric
Copy link
Contributor

Your suggestion would add the need of many smaller enums. A single match would just coerce multiple variants into a single category :

match code {
InvalidUTF8 | InvalidBOM => Encoding, 
UnexpectedTab | TooManySpaces => Indentation 
...
}

@eirnym
Copy link

eirnym commented Jun 27, 2025

I propose following error structure:

ScanErrorCode -> ~> actual error

In this way a person who handle this can stop:

  • at ScanErrorCode
  • at a category level
  • at actual error

Advantages are:

  • It won't be a huge single list of error codes, so it's easier to document and maintain in the future
  • Describe category is basically free in terms of memory and call sequence

As I understand (please correct me if I wrong), you propose to have a one big enum and call a function to get a category. This isn't cheap and isn't free.

@eirnym
Copy link

eirnym commented Jul 5, 2025

@Ethiraric one thing I haven't address in my previous comment is amount of categories. There won't be many categories in the first place and category amount won't grow. Additionally, I doubt that classification would need to be ever changed

@eirnym
Copy link

eirnym commented Jul 12, 2025

it's not my project, but I've tried to make it more readable, because such enums are very hard to ever change. you can decide whatever you see fit.

Co-authored-by: Ethiraric <ethiraric@gmail.com>
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.

Error enum

3 participants