elm / url

Build and parse URLs. Useful for HTTP and "routing" in single-page apps (SPAs)
https://package.elm-lang.org/packages/elm/url/latest/
BSD 3-Clause "New" or "Revised" License
74 stars 43 forks source link

Builder should return Url #23

Closed j-maas closed 5 years ago

j-maas commented 5 years ago

I expected the Url.Builder to create Urls, not Strings. It loses all guarantees that I expected from it.

If it would create a Url, I could simply run Url.toString on it. The reverse is much more hassle, since Url.fromString may produce Nothing, even though I just used the Url.Builder that should prevent me from generating invalid Urls.

Maybe I missed something and I see that changing it means breaking the interface. But I think this is an important consideration.

j-maas commented 5 years ago

Upon further playing with the library, I feel that Url work's differently from what I expected. I will close this issue for now until I come up with a more thought-out proposal.

jcornaz commented 4 years ago

@Y0hy0h Can you reopen this issue please?

Either, the builder must be modified and returns Url, making it less astonishing more type safe. Or, a good explanation should be given and written on the documentation of this library.

Either way, something must be done IMO.

j-maas commented 4 years ago

I closed this issue because I realized that making the library more typesafe would require changes in many functions. It simply is currently oriented towards URL as Strings.

If you have a good suggestion of how to fix this, I propose that you open a new issue where you propose a solution or provide more details to what you see as the problem.

jcornaz commented 4 years ago

I closed this issue because I realized that making the library more typesafe would require changes in many functions. It simply is currently oriented towards URL as Strings.

Yes, It would require many breaking changes. That doesn't mean the changes aren't good or not needed.

"typesafe" is the biggest selling point of elm. It makes total sense to pursue this goal, and improve what can be made more typesafe.

If you have a good suggestion of how to fix this, I propose that you open a new issue where you propose a solution or provide more details to what you see as the problem.

I don't understand. I propose no more nor less than: "Make builders return Url"

If (and only if) an issue is open and accepted, I may propose more in depth solution in the form of a pull request. But I won't start writing code and open a pull request without having an accepted open issue.

j-maas commented 4 years ago

If (and only if) an issue is open and accepted, I may propose more in depth solution in the form of a pull request. But I won't start writing code and open a pull request without having an accepted open issue.

That's a good approach! My point is more that I'm not willing to push my issue further and I would appreciate it if you opened a new one, so that it is clear that you posed the issue. :)

slissner commented 3 years ago

I second @jcornaz in his request.

Currently, I am running in the situation where I am retrieving a Maybe Url form a URI string whereas I would be required to provide a Maybe.withDefault fallbackUrl. Yet, I cannot create simply a fallbackUrl. I would be required to resort to populate the following struct manually.

type alias Url =
    { protocol : Protocol
    , host : String
    , port_ : Maybe Int
    , path : String
    , query : Maybe String
    , fragment : Maybe String
    }

That is quite ugly.

All problems would be solved if Url.Builder would return a Url type that I can simply throw into toString.

Maybe we could reopen the issue, please.

j-maas commented 3 years ago

As I hinted at, I would encourage you to open new issues. This is what the Elm creator, Evan Czaplicki, recommends, so this probably also applies to this repository.

evancz commented 3 years ago

I think functionality like this is best explored in external packages for now.