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: Implement sourcemap composition #67

Merged
merged 23 commits into from
Aug 1, 2023
Merged

feat: Implement sourcemap composition #67

merged 23 commits into from
Aug 1, 2023

Conversation

loewenheim
Copy link
Contributor

No description provided.

@loewenheim loewenheim changed the title WIP: feat: Implement sourcemap composition feat: Implement sourcemap composition Jul 25, 2023
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

only a partial review. this is very hard to digest.

how does it compare with other imlementations that exist in the JS ecosystem?

src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor Author

only a partial review. this is very hard to digest.

I know. Thank you for your comments :)

how does it compare with other imlementations that exist in the JS ecosystem?

I'll do my best to understand at least remapping and compare them. So far I haven't found it easy to understand the code.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I believe the implementation is good, it just needs some better naming and docs.

Instead of left/right, give the two sourcemaps a descriptive name.
We have a very narrow usecase here, we are just injecting a snippet into an already sourcemapped file. Or are we creating a generic solution?

@loewenheim
Copy link
Contributor Author

You're right that the use case is very narrow. I'll try to think of more descriptive names!

src/types.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
@loewenheim
Copy link
Contributor Author

This PR now includes test cases for debug id injection (the intended use case) as well as a property-based test showing the following:
If we edit a string s into t (with sourcemap m₁: t -> s) and then edit t into u (with sourcemap m₂: u -> t), then SourceMap::adjust_mappings(m₁, m₂) is the same sourcemap that we would get if we did the edits from s to u in one go, with the following restriction: The first edits (from s to t) must only happen within lines, and the second edits (from t to u) must only happen to whole lines.

let line_offset = line * 11;
match self {
FirstEdit::Insert(col, s) => {
ms.append_left(line_offset as u32 + *col, s).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Dereferencing in math operations is always so ugly 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of the papercuts of Rust :/

@loewenheim loewenheim merged commit 2ae35b9 into master Aug 1, 2023
4 checks passed
@loewenheim loewenheim deleted the feat/compose branch August 1, 2023 15:26
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