Skip to content

Url parser extensions#2

Open
archerD wants to merge 12 commits into
masterfrom
urltesting
Open

Url parser extensions#2
archerD wants to merge 12 commits into
masterfrom
urltesting

Conversation

@archerD

@archerD archerD commented Oct 28, 2020

Copy link
Copy Markdown

We extend the url parser to handle missing authorities, and empty usernames, passwords, and ports.
Also sanitychecks the range of the port and extends the prettyprinter to print urls without authorities properly.
Also added some test cases for the aforementioned extensions to the url parser.

We welcome any feedback you might have.

Comment thread basis/urls/urls.factor Outdated
":" split1-last [ url-decode ]
[ dup [ string>number [ malformed-port ] unless* ] when ] bi*
[ dup [ dup empty? [ drop f ] [ string>number [
malformed-port ] unless* dup 65535 > over 0 < or [

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I might clean this up a little by putting on multiple lines, something like:

    [
        ":" split1-last [ url-decode ] [
            [ f ] [
                string>number [ malformed-port ] unless*
                dup 0 65535 between? [ malformed-port ] unless
            ] if-empty
        ] bi*
    ] [ f f ] if*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

While the maximum number of ports is 65535 for IPV4 and IPV6 (currently), I'm not sure this limitation should maybe apply to all URLs? Is that what the test suite you are comparing it to does?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's a good point, according to this rfc: https://tools.ietf.org/html/rfc1475#section-4.2 later versions of IP allow for ports higher than 65k. We'll remove this checking, as we would still want to support IPvX.

Comment thread basis/urls/urls.factor Outdated
: unparse-protocol ( url -- )
dup protocol>> [
% "://" % unparse-host-part
protocol>> [

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Now that the code isn't nested, these probably can each be one-line:

protocol>> [ % ":" % ] when*

dup host>> [ "//" % unparse-host-part ] [ drop ] if

@mrjbq7

mrjbq7 commented Oct 28, 2020

Copy link
Copy Markdown

Cool! Should probably open this PR on the factor/factor repo so we can merge it.

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.

3 participants