Skip to content

feat: add FilePath.handle#9065

Merged
magnus-madsen merged 17 commits intoflix:masterfrom
sockmaster27:handler
Nov 3, 2024
Merged

feat: add FilePath.handle#9065
magnus-madsen merged 17 commits intoflix:masterfrom
sockmaster27:handler

Conversation

@sockmaster27
Copy link
Copy Markdown
Contributor

Fixes #9032

@sockmaster27
Copy link
Copy Markdown
Contributor Author

@magnus-madsen
Is this the correct approach

@magnus-madsen
Copy link
Copy Markdown
Member

@sockmaster27
Copy link
Copy Markdown
Contributor Author

@magnus-madsen
So like this?

@magnus-madsen
Copy link
Copy Markdown
Member

Approximately like so:

    pub def handle(f: a -> b \ ef): a -> Result[IOError, b] \ (ef - FilePath) + IO = x ->
        try {
            Ok(f(x))
        } with FilePath {
            def exists(f, k) = {
                try {
                    k(Files.exists(Paths.get(f), (...{}: Vector[LinkOption])))
                } catch {
                    case ex: IOException => Err(IOError.Generic(ex.getMessage()))
                }
            }
        }

Note also the use of IOException.

@sockmaster27
Copy link
Copy Markdown
Contributor Author

Approximately like so:

    pub def handle(f: a -> b \ ef): a -> Result[IOError, b] \ (ef - FilePath) + IO = x ->
        try {
            Ok(f(x))
        } with FilePath {
            def exists(f, k) = {
                try {
                    k(Files.exists(Paths.get(f), (...{}: Vector[LinkOption])))
                } catch {
                    case ex: IOException => Err(IOError.Generic(ex.getMessage()))
                }
            }
        }

Note also the use of IOException.

Wouldn't this catch other unrelated exceptions thrown by the continuation?

@sockmaster27
Copy link
Copy Markdown
Contributor Author

Note also the use of IOException.

What about the InvalidPathException?

@magnus-madsen
Copy link
Copy Markdown
Member

Note also the use of IOException.

What about the InvalidPathException?

Good catch, add it too.

@sockmaster27
Copy link
Copy Markdown
Contributor Author

Actually, it looks like this doesn't throw an IOException at all 😵‍💫.
However, Files.exists throws a SecurityException.

@magnus-madsen
Copy link
Copy Markdown
Member

We ignore security exceptions. The Files class throws IOExs on general.

@magnus-madsen
Copy link
Copy Markdown
Member

Anyway, I think you look at each Files.X op, add its exceptions (except for SecurityEx) plus the InvalidPath thing and that is what you should catch.

@magnus-madsen
Copy link
Copy Markdown
Member

So with that I think it should be possible to fill out this handler. And then we can iterate.

@sockmaster27
Copy link
Copy Markdown
Contributor Author

Approximately like so:

    pub def handle(f: a -> b \ ef): a -> Result[IOError, b] \ (ef - FilePath) + IO = x ->
        try {
            Ok(f(x))
        } with FilePath {
            def exists(f, k) = {
                try {
                    k(Files.exists(Paths.get(f), (...{}: Vector[LinkOption])))
                } catch {
                    case ex: IOException => Err(IOError.Generic(ex.getMessage()))
                }
            }
        }

Note also the use of IOException.

Wouldn't this catch other unrelated exceptions thrown by the continuation?

What about this?

@magnus-madsen
Copy link
Copy Markdown
Member

That's ok for now. We will deal with that later when we add support for java exceptions to the effect system.

@sockmaster27
Copy link
Copy Markdown
Contributor Author

@magnus-madsen
This good?

@magnus-madsen
Copy link
Copy Markdown
Member

Don't extend IOError. We can do it later, but doing it now causes breakages.
Second, no reason to use varargs where there is no problem. (The problem is only if the varargs array type is an interface).

@sockmaster27
Copy link
Copy Markdown
Contributor Author

I assume it's okay now?
Then I will start to implement it for the other operations

@sockmaster27
Copy link
Copy Markdown
Contributor Author

I assume handling OutOfMemoryException is also out of scope?

@magnus-madsen
Copy link
Copy Markdown
Member

I assume it's okay now? Then I will start to implement it for the other operations

Please go ahead.

@magnus-madsen
Copy link
Copy Markdown
Member

@sockmaster27 This looks almost ready? If the list() operation is giving trouble, we could defer it.

@sockmaster27
Copy link
Copy Markdown
Contributor Author

I got list to work by using the old java.io.File. I guess we might be able to fix it later, but this should work for now.

@magnus-madsen
Copy link
Copy Markdown
Member

I got list to work by using the old java.io.File. I guess we might be able to fix it later, but this should work for now.

That's acceptable. We prefer nio, but its totally OK to use file if necessary. Similarly to using FileReader etc.

@sockmaster27 sockmaster27 marked this pull request as ready for review November 3, 2024 11:10
@magnus-madsen
Copy link
Copy Markdown
Member

Looks good: I suggest to next duplicate and split or split and duplicate, if you prefer.

@magnus-madsen
Copy link
Copy Markdown
Member

magnus-madsen commented Nov 3, 2024

In parallel you can also start looking into the Http effect. The design there is less clear, though.

@magnus-madsen magnus-madsen added this pull request to the merge queue Nov 3, 2024
Merged via the queue into flix:master with commit 6753221 Nov 3, 2024
@sockmaster27 sockmaster27 deleted the handler branch November 3, 2024 11:38
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.

Implement handler for FilePath

2 participants