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

Added writeUtf8 and writeUtf8Lines #3167

Merged
merged 8 commits into from
Mar 20, 2023
Merged

Added writeUtf8 and writeUtf8Lines #3167

merged 8 commits into from
Mar 20, 2023

Conversation

TonioGela
Copy link
Member

Hello!
That's my first contribution to fs2, so I hope everything is where it should be.
I added two helpers to Files[F] that mirror the ones already present for readUtf8 and readUtf8Lines, shamelessly stealing the idea from this @armanbilge comment.
I haven't found any unit tests for readUtf8 and readUtf8Lines but for their components, and since writeUtf8 and writeUtf8Lines are built on the same ones, I haven't added any.

It may be worth adding these new combinators to fs2 2.5.x and adding an example like Stream.emit("foo").through(Files[IO].writeUtf8Lines(Path(...))) to the documentation.

@armanbilge
Copy link
Member

It may be worth adding these new combinators to fs2 2.5.x

FS2 2.5.x is EOL (like CE 2.x series). I can't remember when we last merged a PR there :)

adding an example like Stream.emit("foo").through(Files[IO].writeUtf8Lines(Path(...))) to the documentation.

Sure! There may already be an existing example that can be made less verbose with the new helper :)

@TonioGela
Copy link
Member Author

TonioGela commented Mar 19, 2023

FS2 2.5.x is EOL (like CE 2.x series). I can't remember when we last merged a PR there :)

I got distracted by: https://github.com/typelevel/fs2/blob/main/.github/pull_request_template.md?plain=1#L3

Sure! There may already be an existing example that can be made less verbose with the new helper :)

I'll keep these changes in a separate PR to add them once these features are released

* using the specified flags to open the file.
*/
def writeUtf8Lines(path: Path, flags: Flags): Pipe[F, String, Nothing] = in =>
in.through(text.lines).through(writeUtf8(path, flags))
Copy link
Contributor

Choose a reason for hiding this comment

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

As @armanbilge mentioned, if the goal here is to insert a newline between strings, that would be in => writeUtf8(path, flags)(text.intersperse("\n")

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, does that add a newline at the end? Seems like each string should terminate like that, so that ultimately the file terminates in a newline.

Also, does text.lines strip newlines? The reason I ask is because ideally these methods would be inverses of each other.

Copy link
Member Author

@TonioGela TonioGela Mar 19, 2023

Choose a reason for hiding this comment

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

I cried for almost 1 hour in front of failing unit tests and then I saw it:

def writeUtf8Lines(path: Path): Pipe[F, String, Nothing] = writeUtf8(path, Flags.Write)
                                                                    ^

Thank god you pushed for them @armanbilge :D

For what is worth, I think that the logic might take in consideration being on windows probably: text.lines implementation takes in consideration \r too.

Not sure, does that add a newline at the end? Seems like each string should terminate like that, so that ultimately the file terminates in a newline.

Can we make it pluggable with a parameter maybe?

Also, does text.lines strip newlines? The reason I ask is because ideally these methods would be inverses of each other.

It does

Copy link
Member Author

@TonioGela TonioGela Mar 19, 2023

Choose a reason for hiding this comment

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

Also, does text.lines strip newlines? The reason I ask is because ideally these methods would be inverses of each other.

Now that I'm thinking about it, is it the case to add roundtrip property-based tests like 👇 ?

forAll { xs: List[String] =>
  Stream.emits(xs).through(Files[IO].writeUtf8Lines(path)) ++
      Files[IO].readUtf8Lines(path).compile.toList.assertEquals(xs)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

For what is worth, I think that the logic might take in consideration being on windows probably: text.lines implementation takes in consideration \r too.

Now it's handled

* using the specified flags to open the file.
*/
def writeUtf8Lines(path: Path, flags: Flags): Pipe[F, String, Nothing] = in =>
in.intersperse(lineSeparator).through(writeUtf8(path, flags))
Copy link
Member

@armanbilge armanbilge Mar 19, 2023

Choose a reason for hiding this comment

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

I really feel like we should be appending a line-separator to each string, instead of interspersing.

A sequence of zero or more non- characters plus a terminating <newline> character.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206

Copy link
Member Author

@TonioGela TonioGela Mar 19, 2023

Choose a reason for hiding this comment

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

Would in.flatMap(s => Stream(s, lineSeparator)) do?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly!

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@mpilquist mpilquist merged commit c69ce10 into typelevel:main Mar 20, 2023
@TonioGela TonioGela deleted the files_writeUtf8 branch March 20, 2023 12:53
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.

4 participants