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

ByteString support #19

Closed
hasufell opened this issue Apr 5, 2016 · 6 comments
Closed

ByteString support #19

hasufell opened this issue Apr 5, 2016 · 6 comments

Comments

@hasufell
Copy link
Contributor

hasufell commented Apr 5, 2016

String is a dangerous type when dealing with low-level functions like the ones from the unix file package. It's debatable whether it's a good idea at all to represent paths by Strings, since they depend on semi-random encoding, which can lead to subtle bugs.

Since packages like unix already provide safe ByteString variants like System.Posix.Files.ByteString it would make sense that a low-level library like hinotify also allows to deal with filepaths as ByteStrings.

I've made an attempt (https://github.com/hasufell/hinotify/commit/19094dd59e76e163ec27f5eb5c02b2906dc72bc1) to do so and it works relatively easy if you just do it for addWatch.

The only problem is how you want to structure your modules to avoid code duplication. I've tried to come up with a module named Internal which uses polymorphic types to work around this, but it's not that pretty and it still changes the exposed API, which is bad, see https://github.com/hasufell/hinotify/commit/5be3709f7429e252ab4dc8c0e5bcaee4fb476b1f

Any idea on how to structure things here or can we just do an initial copy-paste and then maintain both modules at the same time?

@kolmodin
Copy link
Owner

Using String is indeed not great, but we've worked to remove the encoding issues. It's best not to use String at all, it has very few legitimate use cases.

Text is an interesting choice, though since it's internally UTF-16 there will be a lot of conversions back and forth to UTF-8.

ByteString might be what we end up with, though semantically it`s also not correct since it's payload is just bytes, not UTF-8.

I need to look into that a bit further and what to pick. Whatever we pick, that will be the only API. Having all code duplicated, like in #20, is more bug prone than just having the String API.

@hasufell
Copy link
Contributor Author

hasufell commented Apr 19, 2016

ByteString might be what we end up with, though semantically it`s also not correct since it's payload is just bytes, not UTF-8.

Well, I think it's more about the fact that we interact with readdir on the lower level, which is what gives us the true filenames of directory contents. And they are just C chars. So the idea is not to mess with that for as long as possible. When it's about user input... it's always another problem.

ByteString so far is the only alternative to String that some libraries provide for filepath handling. It's pretty much the only reasonable alternative ecosystem-wide, IMO.

@hasufell
Copy link
Contributor Author

hasufell commented Apr 21, 2016

The Abstract FilePath proposal also indicates that if we ever get a solution, the representation will probably be ByteString or something very similar to ByteString.

However, I think it doesn't really make sense to wait for this (if it ever happens). If you don't want to switch to ByteString and don't want to maintain two modules, I can just maintain a hinotify-bytestring package at hackage.

@hasufell
Copy link
Contributor Author

hasufell commented May 1, 2016

I'm going for a fork for now.

@hasufell hasufell closed this as completed May 1, 2016
@hasufell
Copy link
Contributor Author

hasufell commented Jun 2, 2016

It seems you switched to ByteString, nice. We can probably deprecate hinotify-bytestring after you've done a new release.

@kolmodin
Copy link
Owner

kolmodin commented Jun 3, 2016

Right. Using ByteString is just one of the missing features of this library. I'd also like to change the API #4 so that you're not required to use threads for dispatching of work.
Probably these things should be released in separate releases.

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

No branches or pull requests

2 participants