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

Percent encode query #214

Closed
wants to merge 3 commits into from
Closed

Conversation

turboMaCk
Copy link
Member

@turboMaCk turboMaCk commented Oct 27, 2020

resolves #200

This PR has 2 commits:

  1. Fixes the issue itself
  2. Unification of createUrl function in search with routeToString to ensure compatibility of urls

Showcase

nix-search

elm.json Outdated Show resolved Hide resolved
Comment on lines +24 to +30
{-| Fixes issue with elm/url not properly escaping string
-}
queryString : String -> Query.Parser (Maybe String)
queryString =
Query.map (Maybe.andThen Url.percentDecode) << Query.string


Copy link
Member Author

Choose a reason for hiding this comment

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

Since Domen's PRs are still not merged we need to define fixed version ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

💯

Comment on lines +34 to +36
[ Url.Parser.map Home Url.Parser.top
, Url.Parser.map NotFound (Url.Parser.s "not-found")
, Url.Parser.map Packages
Copy link
Member Author

Choose a reason for hiding this comment

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

I took a liberty and reformatted these parts a bit. This is all elm-format compatible.

Comment on lines +89 to +93
{-| Fixes issue with elm/url not properly escaping string
-}
builderString : String -> String -> QueryParameter
builderString name =
Builder.string name << Url.percentEncode
Copy link
Member Author

Choose a reason for hiding this comment

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

Since Domen's PRs are still not merged we need to define fixed version ourselves.

Copy link
Member

Choose a reason for hiding this comment

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

💯

src/Route.elm Outdated
in
"/" ++ String.join "/" path ++ "?" ++ String.join "&" (List.filterMap Basics.identity query)
routeToString =
uncurry Builder.absolute << routeToPieces
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only use of uncurry from basics-extra

Comment on lines +301 to +303
createUrl toRoute channel query show from size sort =
toRoute (Just channel) query show (Just from) (Just size) (Just <| toSortId sort)
|> Route.routeToString
Copy link
Member Author

Choose a reason for hiding this comment

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

unification of URL building logic so we can keep fixes in single place.

Copy link
Member

Choose a reason for hiding this comment

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

🥇

@@ -320,6 +313,7 @@ type Channel
| Release_20_03
| Release_20_09


Copy link
Member Author

Choose a reason for hiding this comment

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

elm format.

@@ -102,7 +103,7 @@ update navKey msg model =
let
( newModel, newCmd ) =
Search.update
"options"
Route.Options
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@garbas garbas left a comment

Choose a reason for hiding this comment

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

I've added the feedback. I'm closing the PR since @turboMaCk will push the branch to repo itself so we get nice previews.

import Url.Parser exposing ((<?>))
import Url.Parser.Query
import Url.Parser.Query as Query
Copy link
Member

Choose a reason for hiding this comment

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

I like to avoid using aliases and prefer to use absolute names.

I know it makes this bigger, but I find it much nicer when reading the code. Especially after some time, since I don't have to look at the header where the function is coming from.

If you don't have strong feelings about this I would like to revert this.

Comment on lines +89 to +93
{-| Fixes issue with elm/url not properly escaping string
-}
builderString : String -> String -> QueryParameter
builderString name =
Builder.string name << Url.percentEncode
Copy link
Member

Choose a reason for hiding this comment

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

💯

Comment on lines +301 to +303
createUrl toRoute channel query show from size sort =
toRoute (Just channel) query show (Just from) (Just size) (Just <| toSortId sort)
|> Route.routeToString
Copy link
Member

Choose a reason for hiding this comment

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

🥇

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.

URL query encoding of spaces is broken
2 participants