Skip to content

More links#584

Merged
mattias-p merged 2 commits into
zonemaster:developfrom
mattias-p:links
Mar 12, 2018
Merged

More links#584
mattias-p merged 2 commits into
zonemaster:developfrom
mattias-p:links

Conversation

@mattias-p

Copy link
Copy Markdown
Member

More link fixing. Using standard autolinks simplifies automatic link checking.

@matsduf matsduf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Github seems to render URLs with or withoud surrounding "<...>". Most of your proposed changes are just adding "<...>". Why do you want to do that?

If we add them here, I think we should do it consistently and having it documented in the "Style Guide".

I select "Request changes" but no necessary to your proposal, but rather to the description of this PR and a reply to this.

@mattias-p

mattias-p commented Mar 8, 2018

Copy link
Copy Markdown
Member Author

The purpose is to increase the range of automatic link checking to include autolinks. Standard Commonmark parser libraries are able to extract standard autolinks but not GFM autolinks. Those libraries are used by link checking tools.

I occasionally use tools to look for broken links. That's how I found the broken links in #518. Unfortunately the tool I used didn't know about the GFM autolink extension, so there were lots of links that didn't get checked that time around. Among them the two links to http://www.zonecheck.fr/features.shtml that are fixed in this PR.

To mitigate the problem of overlooked links in the future I searched for GFM autolinks in zonemaster and added angle brackets to all the links I could find.

I agree that an item about this should be added to the style guide.

@matsduf

matsduf commented Mar 8, 2018

Copy link
Copy Markdown
Contributor

I want to see a more complete proposal of how links should be written Zonemaster documentation before this PR is merged. When I test, putting "<...>" around a link does not add anything. It does not change the parsing.

@mattias-p

mattias-p commented Mar 9, 2018

Copy link
Copy Markdown
Member Author

This is not about presentation on Github. Github renders Commonmark autolinks and GFM autolinks identically.
This is about using tools to go over all the links in an entire repository to verify that they all still work properly. Since GFM autolinks is an extension to Commonmark, they are unlikely to be recognized by Commonmark parsing libraries, and this is why I missed lots of links when I believed I'd checked them all.

I suggest autolinks are entered like this: http://spec.commonmark.org/0.28/#autolinks

@matsduf

matsduf commented Mar 9, 2018

Copy link
Copy Markdown
Contributor

According to http://spec.commonmark.org/0.28/#autolinks the following three should result in autolinks but only the first does in Github:

  1. http://foo.bar.baz
  2. a+b+c:d
  3. localhost:5001/foo

Am I doing something wrong?

@mattias-p

Copy link
Copy Markdown
Member Author

Looks like you've discovered two Github bugs.

They're autolinks according to GFM as well:

And for reference: https://github.github.com/gfm/#what-is-github-flavored-markdown-

@sandoche2k

Copy link
Copy Markdown
Contributor

Except for the confusion of the links <..>, i am ok with the content

@matsduf matsduf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is no harm to have the "<...>" in the already explicit URLs.

@sandoche2k

Copy link
Copy Markdown
Contributor

@mattias-p then pls. merge

@mattias-p mattias-p merged commit a181a4a into zonemaster:develop Mar 12, 2018
@mattias-p mattias-p deleted the links branch March 12, 2018 16:05
@sandoche2k

Copy link
Copy Markdown
Contributor

@mattias-p please close

@mattias-p

Copy link
Copy Markdown
Member Author

@sandoche2k This is already merged.

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