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

Parse autolinks for urls and emails. #11299

Merged
merged 1 commit into from
Jan 21, 2017
Merged

Conversation

hayd
Copy link
Member

@hayd hayd commented May 16, 2015

fixes #9564

cc @one-more-minute

@hayd
Copy link
Member Author

hayd commented May 20, 2015

I forgot that this fails due to @ being escaped. It's probably safe to remove that from the chars we escape (although perhaps the mailto link still works?? - I'm not sure).

Unfortunately/interestingly I don't think commonmark implementation actually follows it's spec with regards to scheme-sensitivity. I don't think we're following the spec on what/when to escape chars... similarly the commonmark implementation is also not following the spec there either!

@MikeInnes MikeInnes self-assigned this May 20, 2015
@MikeInnes
Copy link
Member

Sorry for taking a ridiculously long time to look at this. Looks good anyway. I don't mind removing @ from the escapes to fix this. The only other thing I'd say to to read until >, [space] or \n – for the sake of efficiency it makes sense to stop reading the stream as early as possible.

@hayd hayd force-pushed the autolinks branch 2 times, most recently from 94c81f9 to 55879de Compare September 17, 2015 22:06
@StefanKarpinski StefanKarpinski added the docsystem The documentation building system label Sep 13, 2016
@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Sep 13, 2016
@tkelman
Copy link
Contributor

tkelman commented Dec 15, 2016

Has this been done in Documenter?

@MichaelHatherly
Copy link
Member

Has this been done in Documenter?

Needs to be part of Base.Markdown, so would be nice to get this rebased and finished up (if @hayd has spare time to do it?)

@hayd
Copy link
Member Author

hayd commented Dec 15, 2016

The rebase was over footnotes, seemed to be trivial. Pushed but haven't tested...

I'm yet to action on:

The only other thing I'd say to to read until >, [space] or \n – for the sake of efficiency it makes sense to stop reading the stream as early as possible.

What's a clean way to do that?

@MichaelHatherly
Copy link
Member

What's a clean way to do that?

Does Markdown.startswith (with a Regex as the second argument) do what you need?

Copy link
Contributor

@mpastell mpastell left a comment

Choose a reason for hiding this comment

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

I added a suggestion for reading. It would nice to have this merged for 0.6

function autolink(stream::IO, md::MD)
withstream(stream) do
startswith(stream, '<') || return
url = readuntil(stream, '>')
Copy link
Contributor

Choose a reason for hiding this comment

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

Try url = readuntil(stream, '>', match = ' ')

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we hitting this definition already?

function readuntil(stream::IO, delimiter; newlines = false, match = nothing)
withstream(stream) do
buffer = IOBuffer()
count = 0
while !eof(stream)
if startswith(stream, delimiter)
if count == 0
return String(take!(buffer))
else
count -= 1
write(buffer, delimiter)
continue
end
end
char = read(stream, Char)
char == match && (count += 1)
!newlines && char == '\n' && break
write(buffer, char)
end
end
end

I guess I haven't fully grokked what match/count do there, but it looked to me like it was suggesting you'd use < as the match character i.e. matching braces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I misread it. I think you're right and need to do:

text = readuntil(stream, '>', match = '<')

I originally read it too quickly and thought setting it to ' ' to would stop reading at whitespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think readuntil returns nothing if the input doesn't have '>'. Try with e.g. Markdown.parse("<link"). Try to add url ≡ nothing && return

@StefanKarpinski
Copy link
Member

Bump, would love to have this. Anyone know what's busted on CI here?

@tkelman
Copy link
Contributor

tkelman commented Jan 4, 2017

failed doc build on travis and failed tests on appveyor, due to the _is_link function somehow getting a nothing input?

@hayd
Copy link
Member Author

hayd commented Jan 4, 2017

Returns nothing and resets the stream if delim is not found.

So that's a bug, it should be returning false if url is nothing. Perhaps implementation changed somewhere down the line, at any rate will push a fix this evening.

@hayd
Copy link
Member Author

hayd commented Jan 9, 2017

This is now green.

end
end

# This list is take from the commonmark spec
Copy link
Contributor

Choose a reason for hiding this comment

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

is taken

'<' in s && return false

m = match(r"^(.*)://(\S+?)(:\S*)?$", s)
m == nothing && return false
Copy link
Contributor

Choose a reason for hiding this comment

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

=== nothing

Remove `@` from escaped html.
@tkelman
Copy link
Contributor

tkelman commented Jan 19, 2017

Remind me to merge this in a day or two if we haven't gotten any other feedback.

@KristofferC
Copy link
Member

KristofferC commented Jan 19, 2017

You should merge this in a day or two if we don't get more feedback.

@tkelman
Copy link
Contributor

tkelman commented Jan 21, 2017

I meant more like now.

@tkelman tkelman merged commit a8734dc into JuliaLang:master Jan 21, 2017
@hayd hayd deleted the autolinks branch January 22, 2017 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docsystem The documentation building system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown fails to parse Autolinks
7 participants