Skip to content

Conversation

@CyberShadow
Copy link
Member

No description provided.

Partial fix for Issue 17680.
Partial fix for Issue 17680.
@CyberShadow CyberShadow added the Review:Trivial typos, formatting, comments label Jul 26, 2017
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close Bugzilla Description
17680 Broken ddmd source links in documentation

@WalterBright
Copy link
Member

I used the explicit links in the source code because my text editor makes them clickable links and that is useful when editing the source code, so I object to using a macro to generate it.

@CyberShadow
Copy link
Member Author

@WalterBright DMDSRC is an existing macro already used in the frontend source files. It was defined in dlang/dlang.org#1196 and added to DMD in #5635 and similar PRs.

It was you who added it to the source files which already have it.

It's fine to change your mind about something, but, at least, please don't contradict your own actions and blame me for trying to be consistent with your previous decisions.

Shall we replace all of its instances with full links and get rid of the macro instead, then?

@CyberShadow
Copy link
Member Author

my text editor makes them clickable links

Also: not going to work because of the underscore anti-feature.

@WalterBright
Copy link
Member

I'm not blaming you, just trying to explain. Yes, I changed my mind on it, because the value of clickable links in the text editor became apparent, and also because the source links are now highly unlikely to change.

The _ prefix bug should probably be fixed in Ddoc. https://issues.dlang.org/show_bug.cgi?id=17697

Yeah, getting rid of the macro for Source: is probably the best idea.

@MartinNowak
Copy link
Member

Checking in links to the master branch are not permanent and a bad practice, they go stale when sources are renamed or moved.

@CyberShadow
Copy link
Member Author

and also because the source links are now highly unlikely to change.

The source links will change at least once more when src/ddmd is renamed to src/dmd according to the current plan.

Checking in links to the master branch are not permanent and a bad practice, they go stale when sources are renamed or moved.

I think this is still the best option we have at our disposal (as updating all links after each release is impractical), except, perhaps, not having a full link at all.

@CyberShadow
Copy link
Member Author

Yeah, getting rid of the macro for Source: is probably the best idea.

Done.

@WalterBright
Copy link
Member

$(LINK2 https://github.com/dlang/dmd/blob/master/src/ddmd/_access.d, _access.d)

In a way, this is a bit silly. Ddoc should automatically recognize URLs and convert them automatically into links, rather than needing to use a macro. This will also resolve the leading _ problem. I'll revise https://issues.dlang.org/show_bug.cgi?id=17697 to reflect this.

@WalterBright
Copy link
Member

Some experimentation with Ddoc shows that if a full URL is written, then the _ prefix processing is not done.

@CyberShadow
Copy link
Member Author

I can try a follow-up PR that removes the underscores from the URLs. Doing it as a separate PR will allow us to see if it actually affects generated HTML.

@CyberShadow
Copy link
Member Author

Ddoc should automatically recognize URLs and convert them automatically into links, rather than needing to use a macro.

For aesthetic reasons it might still be a little preferable to avoid showing the full URL in the docs - instead, just showing the file name (relative to the source root) and linking to the full URL.

@WalterBright
Copy link
Member

it might still be a little preferable to avoid showing the full URL in the docs - instead, just showing the file name (relative to the source root) and linking to the full URL.

I thought about that, and sometimes the algorithm would be right, and often it would be wrong. So I think it best to just do the most straightforward thing, and if the user wants more, he can use $(LINK2 ...).

@WalterBright
Copy link
Member

See #7043

@WalterBright
Copy link
Member

Now that #7043 is merged, the Source: links should be changed to plain URLs.

@CyberShadow
Copy link
Member Author

I thought about that, and sometimes the algorithm would be right, and often it would be wrong. So I think it best to just do the most straightforward thing, and if the user wants more, he can use $(LINK2 ...).

I wasn't suggesting that as something the link detection algorithm would do, just something that we should do here explicitly, using LINK2.

Now that #7043 is merged, the Source: links should be changed to plain URLs.

Why though? Making the full URL visible seems a bit excessive. We already use LINK2 for the Authors and License sections.

@WalterBright
Copy link
Member

Why though?

Because then my text editor can directly open a browser on that link. Otherwise, it's several steps to get there. It's the same reason why bugzilla links in the source code are full URLs. Once my editor started being able to click on URLs, I was surprised at how useful it was. Making them clickable is the whole point of having URLs. It's also why I did this PR:

#7061

The license and author URLs are not particularly useful for people working on the code.

I wasn't suggesting that as something the link detection algorithm would do, just something that we should do here explicitly, using LINK2.

If you want to do that, ok, but I don't see much point to it.

@CyberShadow
Copy link
Member Author

Making the URLs clickable in the editor would mean not using LINK2 because they need underscores otherwise.

It seems to me that the correct answer here is to get rid of the underscore detection (outside $(D ...) blocks), and use LINK2 as before.

@WalterBright
Copy link
Member

WalterBright commented Aug 8, 2017

Making the URLs clickable in the editor would mean not using LINK2 because they need underscores otherwise.

Full URLs in LINK2 (or any macro arguments) do not undergo _ highlighting.

It seems to me that the correct answer here is to get rid of the underscore detection (outside $(D ...) blocks), and use LINK2 as before.

_ detection does not happen for full URLs at all, whether they are inside or outside macros. _ detection happens only before any macro expansion.

It is the correct answer, and it works now, and has for years.

@CyberShadow
Copy link
Member Author

Ah, OK. So we just need to remove the underscores then.

@WalterBright
Copy link
Member

Ah, OK. So we just need to remove the underscores then.

From all full URLs, yes. Apparently, nobody had ever tested them :-(

@CyberShadow
Copy link
Member Author

From all full URLs, yes.

Done.

Apparently, nobody had ever tested them :-(

Yep, I haven't looked at the HTML tester's results after #7043.

@CyberShadow
Copy link
Member Author

It is the correct answer, and it works now, and has for years.

Well, it is a bit weird that http://example.com/_something and ftp://example.com/_something are treated differently right now.

@WalterBright
Copy link
Member

it is a bit weird that http://example.com/_something and ftp://example.com/_something are treated differently right now.

True, and it is a reasonable thing to create a PR about.

@WalterBright
Copy link
Member

Links to http://dlang.org/phobos/ddmd_expression.html and similar would also be good in the header comment.

@CyberShadow
Copy link
Member Author

Done :)

Added them as a regular comment instead of in the DDoc block so the HTML pages don't have a link to themselves.

Anything else?

@CyberShadow CyberShadow force-pushed the pull-20170726-092337 branch from eb0d086 to 8a7acb3 Compare August 8, 2017 15:06
@WalterBright WalterBright merged commit 4d86fcb into dlang:master Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:Trivial typos, formatting, comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants