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

Add serde support #264

Merged
merged 9 commits into from
May 14, 2023
Merged

Add serde support #264

merged 9 commits into from
May 14, 2023

Conversation

gavrilikhin-d
Copy link
Contributor

Need to add serde to MietteDiagnostic too, when #262 is merged

@gavrilikhin-d
Copy link
Contributor Author

Implements #260

/// The total length of the span. Think of this as an offset from `start`.
length: SourceOffset,
/// The total length of the span
length: usize,
}

impl SourceSpan {
/// Create a new [`SourceSpan`].
pub fn new(start: SourceOffset, length: SourceOffset) -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be:

Suggested change
pub fn new(start: SourceOffset, length: SourceOffset) -> Self {
pub fn new(start: SourceOffset, length: usize) -> Self {

But will be breaking change.

SourceOffset may not represent length in no way. This looks more like a range (e.g 3..3)

Copy link
Owner

Choose a reason for hiding this comment

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

Can you make a pr for this once this lands? I'll mark it as breaking but that way we won't forget to do it.

There's a good chunk of desirable breaking changes in the pipeline right now and I think I'm gonna find some time to roll up 6.0 at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@gavrilikhin-d gavrilikhin-d requested a review from zkat May 13, 2023 14:49
@zkat
Copy link
Owner

zkat commented May 13, 2023

Looks like you need to fix a conflict, but also I'm not sure about those other dependencies you bumped? Those seem to be what's causing the CI issues

@zkat
Copy link
Owner

zkat commented May 13, 2023

Also you may as well add serde support to MietteDiagnostic while you're at it

@zkat
Copy link
Owner

zkat commented May 14, 2023

Why do you need to update those dependencies?

@gavrilikhin-d
Copy link
Contributor Author

gavrilikhin-d commented May 14, 2023

I don't need to :) Just noticed they are outdated and thought it may be easy to fix it

This reverts commit f2ef757.
@zkat zkat merged commit c25676c into zkat:main May 14, 2023
11 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

2 participants