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

toString missing #1

Open
MartinSStewart opened this issue May 3, 2020 · 12 comments
Open

toString missing #1

MartinSStewart opened this issue May 3, 2020 · 12 comments

Comments

@MartinSStewart
Copy link
Contributor

Hi, I had a look at the API for this package and was surprised there wasn't a toString: DataUri -> String function to complement the existing fromString function. Are there plans to add one?

@Munksgaard
Copy link
Owner

Hi @MartinSStewart! No, but there's no particular reason why there shouldn't be. Feel free to make a PR if you want to add it :-)

@MartinSStewart
Copy link
Contributor Author

MartinSStewart commented May 3, 2020

I started working on a PR but then realized this was going to be more work than expected. Also I realized for my use case it's going to be too slow to parse the data URIs (I'm working with data URIs that are up to 256kb).

I did notice that you have a lot of unused dependencies in this package so I went ahead and made a PR removing those at least (though I seem to have broken something in CI, let me check on that first).

@Munksgaard
Copy link
Owner

I started working on a PR but then realized this was going to be more work than expected. Also I realized for my use case it's going to be too slow to parse the data URIs (I'm working with data URIs that are up to 256kb).

Interesting. Any ideas how we could improve elm-data-uri to handle these cases, or are you going to use an alternative route to handle your use-case?

@MartinSStewart
Copy link
Contributor Author

MartinSStewart commented May 3, 2020

For my use case, I only need the data URI to be validated efficiently and I don't need to parse any of the information contained in it. I'm just getting a data URI from somewhere and then passing it along.

I suppose one way to make that work would be to add a fromStringLazy: String -> Maybe DataUri which validates the string but doesn't do any parsing until the user tries to read data from it (parsing the mediaType is probably fine though).

@Munksgaard
Copy link
Owner

For my use case, I only need the data URI to be validated efficiently and I don't need to parse any of the information contained in it. I'm just getting a data URI from somewhere and then passing it along.

I suppose one way to make that work would be to add a fromStringLazy: String -> Maybe DataUri which validates the string but doesn't do any parsing until the user tries to read data from it (parsing the mediaType is probably fine though).

That would be cool, but perhaps it would be enough to have a validDataUri : String -> Bool or something?

@MartinSStewart
Copy link
Contributor Author

I'm creating a package and the simplified API looks like this sendData : DataUri -> Cmd Msg. It could be sendData : String -> Cmd Msg but then the user could accidentally send in some nonsense string instead. So I do want to be able to wrap the string in DataUri after it's been validated. That isn't possible if I only have validDataUri : String -> Bool.

@Munksgaard
Copy link
Owner

So you need something like fromStringLazy: String -> Maybe (() -> DataUri)? If you want to add it, you're more than welcome to :-)

@MartinSStewart
Copy link
Contributor Author

I'm not sure that would work. I'd still need to call () -> DataUri (the expensive step) before I could get the DataUri and send it along in an http request.

By fromStringLazy what I meant was something like this

type Data
    = Base64 Bytes
    | Raw String
    | Lazy String

fromStringLazy : String -> Maybe DataUri
fromStringlazy dataUriText =
    if fastValidation dataUriText then
        Just 
            { mediaType = getMediaTypeWhichShouldAlsoBeFast dataUriText 
            , data = Lazy dataUriText 
            }
    else
        Nothing

toString : DataUri -> String
toString dataUri =
    case dataUri.data of 
        Lazy data -> data -- Also fast
        Raw data -> ...
        Base64 data -> ...

@Munksgaard
Copy link
Owner

I see. As long as it doesn't change how the ordinary fromString and parser works, then I'm fine with it.

@MartinSStewart
Copy link
Contributor Author

I'm not actually sure if this is a good idea. Adding a Lazy variant makes Data more annoying to use for anyone else using this package. Even if that's worthwhile, I think benchmarking needs to be done to be sure validation is fast enough and parsing is too slow. That's more work than I have time for so I think (at least for now) I'll just make a DataUri type in my package that doesn't do any parsing or validation and is just to help make sendData : DataUri -> Cmd Msg more understandable.

@Munksgaard
Copy link
Owner

Fair enough.

@Munksgaard
Copy link
Owner

I guess we can leave the issue open, a toString method wouldn't be out of the question.

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