Skip to content

Conversation

@garyb
Copy link
Member

@garyb garyb commented Oct 6, 2017

I would consider the current behaviour to be a bug. Certainly for the query anyway since one way included ? and the other did not - the fragment did not require it in either, but that it was different seemed weird so I thought I'd make it consistent in that regard also. Made the code a bit cleaner as a bonus, reduced some repetition of # printing/parsing in each URI variety.

@garyb garyb requested a review from kritzcreek October 6, 2017 12:23
@paluh
Copy link
Contributor

paluh commented Oct 6, 2017

I think that similar change should be introduced for authority part and // prefix... I've dropped pathy representation on my fork, so I'm not able to create clean pull requests...

@garyb
Copy link
Member Author

garyb commented Oct 6, 2017

@paluh yeah, I was looking at the // part too... I'm not entirely sure whether it should be for authority or "hierarchical part" - the way things are now, it should be for hierpart, but given the inability to express some URLs you were finding I'm not sure if that's right.

@garyb
Copy link
Member Author

garyb commented Oct 6, 2017

Wow, the parsers are really messed up right now, no wonder those current test cases fail 😄 I have some ideas now... sorry for leaving that other thread hanging in which we discussed this stuff @paluh, but I do have some ideas about some fixes now at least!

@paluh
Copy link
Contributor

paluh commented Oct 6, 2017

https://tools.ietf.org/html/rfc3986#section-3.2

The authority component is preceded by a double slash ("//") and is
terminated by the next slash ("/"), question mark ("?"), or number
sign ("#") character, or by the end of the URI.

In case of relative ref I think that for example localhost should be parsed as path in case of relative ref (and unparsable by absolute uri parser - scheme requires :), ///test should be parsed as empty authority and /test path by relative ref parser etc.

So yes // prefix should go in general to authority parser in my opinion.

@paluh
Copy link
Contributor

paluh commented Oct 6, 2017

ABNF and regex from the spec seems to be consistent with this description...

sorry for leaving that other thread hanging

No problem - I know that you are busy ;-)

@garyb
Copy link
Member Author

garyb commented Oct 6, 2017

@paluh thanks for those pointers, now the authority stuff has been untangled the /top_story.htm test case finally works. That's the one case of the current failures that I've wanted to fix for the longest time!

Copy link

@kritzcreek kritzcreek left a comment

Choose a reason for hiding this comment

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

Looks good!


parser Parser AbsoluteURI
parser = AbsoluteURI
<$> (optionMaybe Scheme.parser <* string ":")

Choose a reason for hiding this comment

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

Don't need these parens

@garyb garyb merged commit 6bfb21c into purescript-contrib:master Oct 6, 2017
@garyb garyb deleted the parse-print-tweaks branch October 6, 2017 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants