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

Discussion: Simplify the API for newcomers #77

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

Hombre-x
Copy link
Contributor

@Hombre-x Hombre-x commented Jul 18, 2024

As @TonioGela and @armanbilge suggested, the API is bloated with difficult concepts such as Resources or context abstractions.

To ensure that newcomers are not hindered in their encounter with these things, some deletion - refactoring - changing of the API was suggested to improve the experience with the library in general.

This PR aims to open a discussion to merge some of these changes and further discuss if some more changes are required to accomplish this goal of enhancing the experience of new users.

What has been done so far:

  • Delete the readStream function to get rid of the Stream type and concepts.
  • Squash the WithCharset method with their standalone variant (similar to os-lib) style.
  • Changed the signature of the tempFile and tempDirectory to not return a Resource.
  • Refactored some tests to deal with the new API.

Whats left to discuss:

The As variant of the three standalone methods was unchanged as I think we should talk before If I may get rid only of the typeclass use and instead add the codec as a dependency in the parameters, or, get rid of the methods entirely as well as with its example in the documentation.

Deleted the `readStream` method

Squashed the `readWithCharset` method with `read`
Formatted sources and deleted unused imports
@Hombre-x Hombre-x marked this pull request as draft July 18, 2024 19:19
@TonioGela
Copy link
Contributor

TonioGela commented Jul 25, 2024

The simplification looks good, but I think you can do further, for example:

-def list(path: Path): Stream[IO, Path] = files.list(path)
+def list(path: Path): IO[List[Path]] = files.list(path).compile.toList

-  def open(path: Path, flags: Flags): Resource[IO, FileHandle[IO]] =
-   files.open(path, flags)

-  def readCursor(path: Path, flags: Flags): Resource[IO, ReadCursor[IO]] =
-   files.readCursor(path, flags)

The As variant of the three standalone methods was unchanged as I think we should talk before If I may get rid only of the typeclass use and instead add the codec as a dependency in the parameters, or, get rid of the methods entirely as well as with its example in the documentation.

The thing is that I don't think any newcomer will ever use this methods, nor write a codec for something, BUT if we write a short tutorial on "how to implement your own saving format for your application" I might be in favor of keeping it there. It's not the main goal of this library, but we can discuss it.

Keep up with the good work. I'm willing to also see the site/doc changes that you're doing.
Cheers.

The `load` method now returns an `IO[List[Path]]` instead of a `Stream[IO, Path]`

The `open` and `readCursor` methods where eliminated as well (not going to be used that often anyways), just as suggested.

Co-authored-by: TonioGela <toniogela89@gmail.com>
@Hombre-x Hombre-x marked this pull request as ready for review July 26, 2024 04:15
@zetashift
Copy link
Contributor

zetashift commented Aug 10, 2024

For read weren't we going to default to UTF-8?

EDIT: nvm see it correctly now!

@TonioGela
Copy link
Contributor

TonioGela commented Aug 10, 2024

For read weren't we going to default to UTF-8?

EDIT: nvm see it correctly now!

What have you seen? I'm asking it because I forgot what we decided about it, and forcing users to specify UTF-8 everywhere is a no-go.

@Hombre-x
Copy link
Contributor Author

I guess he saw that the read method wasn't overloaded 🤔

@TonioGela
Copy link
Contributor

I guess he saw that the read method wasn't overloaded 🤔

Okay, but do we have just a single read method now? The one that doesn't default to utf-8? That's a no-go.

@zetashift
Copy link
Contributor

Okay, but do we have just a single read method now? The one that doesn't default to utf-8? That's a no-go.
We default to utf8:

def read(path: Path): IO[String] = files.readUtf8(path).compile.string

I guess he saw that the read method wasn't overloaded 🤔
Yup, exactly what happened :P

Copy link
Member

@lenguyenthanh lenguyenthanh left a comment

Choose a reason for hiding this comment

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

I added a small name-shedding comment, which shouldn't block this pr.
Great works @Hombre-x and everyone!

Now all the tempDirectory and tempFile functions are named withTempDirectory and withTempFile, respectively
@Hombre-x
Copy link
Contributor Author

Names changed 😀

@TonioGela TonioGela merged commit b288088 into davenverse:main Aug 13, 2024
10 checks passed
@TonioGela TonioGela deleted the simpler-api branch August 13, 2024 17:49
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