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

Fix multiline string emit. #8

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

cwize1
Copy link
Contributor

@cwize1 cwize1 commented Jul 4, 2024

Use |- instead of | when there is not a trailing newline in the string value.

Use `|-` instead of `|` when there is not a trailing newline
in the string value.
src/emitter.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@davvid davvid left a comment

Choose a reason for hiding this comment

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

These notes are mostly polish to get it perfect. I don't mind if this is merged as-is, but if these notes are addressed then it'll make it even better. Thanks for getting this working.

src/emitter.rs Show resolved Hide resolved
src/emitter.rs Outdated Show resolved Hide resolved
src/emitter.rs Outdated Show resolved Hide resolved
@Ethiraric
Copy link
Contributor

For some reason, I hadn't noticed you opened a PR on saphyr as well. Thanks! <3

@davvid I ported the few style changes you requested to yaml-rust2.

emitter.dump(original).unwrap();

let documents = Yaml::load_from_str(&emitted).unwrap();
println!("emitted {emitted}");
Copy link
Contributor

Choose a reason for hiding this comment

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

was this a leftover debugging println or is this here for future debugging purposes? just checking in case we should trim it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not against keeping it anyway. They are captured by cargo test and discarded unless the test fails.

Our test suite takes ~0.2s on my machine to run so we can afford a little bit of extra debugging detail should something fail in the future.

@Ethiraric Ethiraric merged commit d9d0f90 into saphyr-rs:master Jul 9, 2024
3 checks passed
@Ethiraric
Copy link
Contributor

Thank you! <3

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.

3 participants