Skip to content

add URL recognition to Ddoc spec #1858

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

Merged
merged 3 commits into from
Jan 25, 2018
Merged

Conversation

WalterBright
Copy link
Member

Document enhancement dlang/dmd#7043

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks!

@PetarKirov
Copy link
Member

FYI @s-ludwig

@s-ludwig
Copy link
Member

s-ludwig commented Aug 8, 2017

@WalterBright: In dlang/dmd#7043 you stated that code like $(LINK http://www.foo.com) won't be affected, but the spec just says that URL recognition happens before macro substitution. I'm missing a link between the two. Is there some intermediate state, where the text is separated into macro and non-macro parts and only the latter are searched for URLs? Also:

  • $(LI This is a http://example.com test) - is the URL recognized there?
  • $(LI URL: http://example.com) - is the trailing ) part of the URL?

@WalterBright
Copy link
Member Author

@s-ludwig The recognition happens on the raw text. Macro invocations are part of that.

$(LI This is a http://example.com test) - is the URL recognized there?

Yes. It must, as the entire Ddoc text is often in a macro.
https://github.com/dlang/dmd/blob/master/src/ddmd/doc.d#L2429

$(LI URL: http://example.com) - is the trailing ) part of the URL?

No. https://github.com/dlang/dmd/blob/master/src/ddmd/doc.d#L1935

@wilzbach
Copy link
Member

wilzbach commented Sep 9, 2017

Rebased and set to auto-merge as this is now the behavior of DMD.

spec/ddoc.dd Outdated
@@ -542,12 +546,23 @@ Identifiers in documentation comments that are function parameters or are
names that are in scope at the associated declaration are emphasized in
the output.
This emphasis can take the form of italics, boldface, a hyperlink, etc.
How it is emphasized depends on what it is - a function parameter, type,
How it is emphasized depends on what it is — a function parameter, type,
Copy link
Member

Choose a reason for hiding this comment

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

There's a problem somewhere around the —, because of which the Latex build fails the following rather cryptic error:


! Misplaced alignment tab character &.
l.24904 ...t is emphasized depends on what it is &
                                                  mdash; a function paramete...

? 
! Emergency stop.

spec/ddoc.dd Outdated
$(P
URLs are sequences of characters starting with 'http://' or 'https://',
continue with one or more characters from the set of letters, digits and
`-_?=%&/+#~.`, and contain at least one `.`.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the best preference here, the easiest way to fix -_?=%&/+#~. is to put in a code block, otherwise sth. like '-_?=%$(AMP)/+#~.' would also work

Copy link
Member

Choose a reason for hiding this comment

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

Code blocks look much cleaner.

@PetarKirov
Copy link
Member

PetarKirov commented Sep 10, 2017

It looks like now the build is failing because it can't reach code.dlang.org. @wilzbach can you rebase again to restart the build?

spec/ddoc.dd Outdated
$(P
URLs are sequences of characters starting with 'http://' or 'https://',
continue with one or more characters from the set of letters, digits and
`-_?=%&/+#~.`, and contain at least one `.`.
Copy link
Member

@MetaLang MetaLang Sep 11, 2017

Choose a reason for hiding this comment

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

This looks very weird in the resulting page. I think it'd be better to spell it out and write ...and contain at least one period.

image

@quickfur
Copy link
Member

quickfur commented Jan 4, 2018

ping @WalterBright Any updates w.r.t. @MetaLang 's request for changes? Let's get this moving along.

@dlang-bot dlang-bot removed the stalled label Jan 5, 2018
@WalterBright
Copy link
Member Author

WalterBright commented Jan 25, 2018

Did @MetaLang 's request

@quickfur
Copy link
Member

Just an afterthought: is there a way to suppress this URL recognition behaviour? Just an example off the top of my head, that perhaps we're documenting a URL parsing library, and the ddoc would like to give examples of URLs or URL snippets. It would be nice if there was a way to distinguish between these example URLs (that we don't want to generate a link for) vs. real URLs that we do want to generate a link for.

@quickfur
Copy link
Member

P.S. In general, I'm wary when something is automated with no way to turn it off. I'm all for smart recognition of URLs, section headings, etc., but there needs to be a way to suppress this behaviour when the automatic behaviour is not desirable. I don't want to see this become yet another train wreck like the auto highlighting of keywords in plain text, that has led to underscore proliferation everywhere in the Phobos docs (and tons of wasted time churning through PRs that do nothing except prefix underscores to words that got highlighted when they shouldn't be).

@MetaLang
Copy link
Member

That's a good point, but I think it's something that can easily be added in a follow-up PR.

$(H3 $(LNAME2 urls, URLs))

$(P
URLs are sequences of characters starting with 'http://' or 'https://',
Copy link
Member

Choose a reason for hiding this comment

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

Here you need to add that the sequences are preceded by whitespace, otherwise asdhttp:// will be considered a URL.

Copy link
Member

Choose a reason for hiding this comment

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

Ironically, github considers the above a URL!

Copy link
Member

Choose a reason for hiding this comment

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

so I guess I'll approve...

@andralex andralex merged commit 75039cf into dlang:master Jan 25, 2018
@andralex
Copy link
Member

Ah, one more thing - the macro should be different from LINK so people can detect and neutralize it if needed. For example it should be DDOC_LINK_AUTODETECT

@quickfur
Copy link
Member

Yes, please make it DDOC_LINK_AUTODETECT. This would provide the escape hatch I was alluding to.

@quickfur
Copy link
Member

Here: #2127

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.

8 participants