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: add simple Position/Span From implementations for LineColLocation #860

Merged
merged 2 commits into from May 26, 2023

Conversation

HoloTheDrunk
Copy link
Contributor

@HoloTheDrunk HoloTheDrunk commented May 26, 2023

What?

Adds From<Position<'_>> and From<Span<'_>> implementations to LineColLocation.

Why?

This will make integrating Pest errors and types into custom error types slightly less verbose.
It's technically possible to create a pest error from a span and then get the LCL from that, but it's an unnecessary overhead.

@HoloTheDrunk HoloTheDrunk requested a review from a team as a code owner May 26, 2023 12:47
@HoloTheDrunk HoloTheDrunk requested review from NoahTheDuke and removed request for a team May 26, 2023 12:47
Copy link
Member

@NoahTheDuke NoahTheDuke left a comment

Choose a reason for hiding this comment

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

Looks good. Can you write a simple test showing these off?

@HoloTheDrunk
Copy link
Contributor Author

HoloTheDrunk commented May 26, 2023

Never bad to have unit tests, though I can't seem to distill the use case where these implementations are important enough to put it in one given that the issue only crops up when making decently complex wrapping error types.
For example, my use case has errors that look like this

Err(Error::new(
    ErrorKind::Code {
        r#type: CodeError::SignatureMismatch(
            parsed.name,
            parsed.signature,
            loaded.signature(),
        ),
        section: Section::Imports,
    },
    lcl_from_bounds(import.as_span().split()), // This is the part where the lack of a From impl became an issue
))

@NoahTheDuke
Copy link
Member

Yeah, I understand how that's hard to distill down for a test. Having a simple unit test works for me, just something to cover the barest case to prevent obvious regressions.

@tomtau tomtau merged commit a4c93a7 into pest-parser:master May 26, 2023
9 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.

None yet

3 participants