radicle-dev / radicle-git

Everything Radicle growing around Git
Other
35 stars 5 forks source link

radicle_surf::diff::Line and newlines #142

Closed slack-coder closed 8 months ago

slack-coder commented 10 months ago

What is the relationship between diff::Line and trailing newlines?

It is hard to tell from the documentation whether they are included, enforced, or optional, when it should communicate what consumers can expect about diff::Lines content.

Edit: update context

FintanH commented 10 months ago

You can see here that trailing newlines are ignored and aren't included as a diff::Line.

slack-coder commented 10 months ago

Its more about diff::Line itself, whether diff::Lines with/without trailing '\n' are legal and expected.

println("{}", line.from_utf8().unwrap());
print("{}", line.from_utf8().unwrap());

or

let line = Line::from("hello".to_string());
let line = Line::from("hello\n".to_string());
FintanH commented 10 months ago

It has no opinion on newlines https://github.com/radicle-dev/radicle-git/blob/main/radicle-surf/src/diff.rs#L452-L480, so they're legal afaict.

slack-coder commented 10 months ago

Good to know, its a potential gotcha for implementers as it can provide lines ending with or without '\n'.

FintanH commented 10 months ago

And why would that be a gotcha?

slack-coder commented 8 months ago

And why would that be a gotcha?

"its a potential gotcha for implementers as it can provide lines ending with or without '\n'." and its not documented for people handling the type. Has this changed for the issue to be marked as completed?