WordPress.org

Making WordPress.org

Opened 5 years ago

Closed 5 years ago

#447 closed enhancement (fixed)

Link to actual source from reference pages

Reported by: DrewAPicture Owned by: coffee2code
Milestone: Priority: normal
Component: Developer Hub Keywords: has-patch
Cc:

Description

Currently, "source" links on reference pages link to the corresponding "source" term archives, and as someone has already suggested for WP-Parser, we should at the very least, link the file and line number in trac browser.

The attached patch renames the get_source_file_link() function to the more apt get_source_file_archive_link() name, which opens the door to using a new version of get_source_file_link() to actually return the trac browser URL with or without the line number appended.

Two other things to note:

  • This reiterates the need to just completely skip duplicate hooks when parsing, as those duplicates can have a tendency to overwrite the documented versions of themselves.
  • In the patch I used a generic "More in this file" string as the anchor label for the source term archive. This is in hesitation to leveraging term labels due to translation issues there cited in core many times. Ideally, of course, I'd prefer something like "More hooks in this file", or "More functions in this file", etc. Might be another ticket for another day.

Attachments (2)

447.diff (3.5 KB) - added by DrewAPicture 5 years ago.
447.png (75.5 KB) - added by coffee2code 5 years ago.
Screenshot demonstrating on-page changes proposed in 447.diff.

Download all attachments as: .zip

Change History (10)

@DrewAPicture
5 years ago

#2 @DrewAPicture
5 years ago

#454 was marked as a duplicate.

@coffee2code
5 years ago

Screenshot demonstrating on-page changes proposed in 447.diff.

#3 follow-up: @coffee2code
5 years ago

I'm on board with the function renaming and repurposing as proposed (with some minor nitpicks*).

I'm not quite in agreement with the front end implementation. Attached is 447.png which demonstrates the changes made in 447.diff.

The "Source:" filename originally/currently points to the source file archive (e.g. https://developer.wordpress.org/reference/files/wp-includes/functions.php). The patch changes that to point to the trac browser view of the function (e.g. https://core.trac.wordpress.org/browser/tags/3.9/src/wp-includes/functions.php#L2873) and adds a new link, "More in this file...", that links to the source file archive.

I prefer the "Source:" link to be unchanged (the source file name linking to the source file archive), and that we add a "View source" link somewhere. Verbiage and location could do with additional input. @sams? @siobhan? If the consensus if for what @DrewAPicture proposed, I'm fine with that.


Nitpicks:

  • I'd prefer to esc_url() late (when get_source_file_link() gets used and not in it)
  • The core.trac link should point to the tagged release and not to trunk, since trunk will always be in flux, likely causing line numbers to not sync up.

#4 in reply to: ↑ 3 ; follow-up: @DrewAPicture
5 years ago

Replying to coffee2code:

I prefer the "Source:" link to be unchanged (the source file name linking to the source file archive), and that we add a "View source" link somewhere. Verbiage and location could do with additional input. @sams? @siobhan? If the consensus if for what @DrewAPicture proposed, I'm fine with that.

Hmm. I like using "View source" for the source browser link but disagree with using "Source" for the "source term". I think it's misleading. What about "Source file" and "View source" or similar?


Nitpicks:

  • The core.trac link should point to the tagged release and not to trunk, since trunk will always be in flux, likely causing line numbers to not sync up.

It was my understanding from multiple conversations that we'd be parsing trunk on a regular basis -- half the reason for parsing docs in the first place. Is that not the case?

#5 in reply to: ↑ 4 ; follow-up: @coffee2code
5 years ago

Replying to DrewAPicture:

Replying to coffee2code:

I prefer the "Source:" link to be unchanged (the source file name linking to the source file archive), and that we add a "View source" link somewhere. Verbiage and location could do with additional input. @sams? @siobhan? If the consensus if for what @DrewAPicture proposed, I'm fine with that.

Hmm. I like using "View source" for the source browser link but disagree with using "Source" for the "source term". I think it's misleading. What about "Source file" and "View source" or similar?

I'm cool with that. However it is labeled, I feel the linked filename should link to the term archive and not the source for the function.


Nitpicks:

  • The core.trac link should point to the tagged release and not to trunk, since trunk will always be in flux, likely causing line numbers to not sync up.

It was my understanding from multiple conversations that we'd be parsing trunk on a regular basis -- half the reason for parsing docs in the first place. Is that not the case?

Hmm, that's not my understanding. Personally I don't feel parsing trunk is worthwhile due to its in-flux nature. Basically if doing so you're documenting code that hasn't been released yet and isn't guaranteed to be released. If we're presenting a single definitive documentation source, the latest release should be it, not trunk.

Why do you feel that parsing trunk is "half the reason for parsing docs in the first place"? Is not documenting the latest release sufficient? It's what most people should be using. Like I said, until a release is made, anything in trunk could be yanked/changed before release.

I'm aware a discussion on another ticket, #331 touched on this and spawned a brief discussion. I agree with @GaryJ and @Rarst there. Trunk isn't stable and shouldn't be the definitive documentation source. I see your point that it'd be nice to incorporate doc updates in core as they happen, but I'm not aware of an effort to differentiate between update-improvements-of-existing-docs and docs-for-newly-introduced-functions. There isn't a current provision to recognize stuff later than a given version so that it could be ignored on the site.

After recent efforts, a lot of what exists has been better documented. And given our fairly quick release cycle nowadays, I don't see the harm in waiting until the next release to have updated inline docs becoming available.

Bear in mind documentation isn't black and white between doc-updates-to-existing-functions and docs-for-new-functions, i.e. using @since as some sort of flag. Existing functions could have existing arguments changed, deprecated, or new arguments added, which would not be reflected in the function's @since.

Maybe this aspect is worth a ticket of its own so it can be debated since it has arisen as tangential discussions for at least two tickets.

#6 in reply to: ↑ 5 @DrewAPicture
5 years ago

Replying to coffee2code:

Replying to DrewAPicture:


Nitpicks:

  • The core.trac link should point to the tagged release and not to trunk, since trunk will always be in flux, likely causing line numbers to not sync up.

It was my understanding from multiple conversations that we'd be parsing trunk on a regular basis -- half the reason for parsing docs in the first place. Is that not the case?

Hmm, that's not my understanding. Personally I don't feel parsing trunk is worthwhile due to its in-flux nature. Basically if doing so you're documenting code that hasn't been released yet and isn't guaranteed to be released. If we're presenting a single definitive documentation source, the latest release should be it, not trunk.

<snip>

I'm aware a discussion on another ticket, #331 touched on this and spawned a brief discussion. I agree with @GaryJ and @Rarst there. Trunk isn't stable and shouldn't be the definitive documentation source. I see your point that it'd be nice to incorporate doc updates in core as they happen, but I'm not aware of an effort to differentiate between update-improvements-of-existing-docs and docs-for-newly-introduced-functions. There isn't a current provision to recognize stuff later than a given version so that it could be ignored on the site.

In the absence of parsing different versions of WordPress separately, e.g. 3.8, 3.9, 4.0, parsing trunk for the overall reference is the preferred alternative. Look at the Drupal API docs. They're parsing and disseminating in-progress docs for Drupal 8 even while Drupal 7.x is the stable release.

Honestly, parsing by version would be ideal -- meaning we'd parse core upon the x.x release and save it, upadting in the event of a point release. Followed by independently parsing trunk as the new x.x+1 release, clearly marking new APIs as experimental and every other page as likely in flux.

<snip>

Bear in mind documentation isn't black and white between doc-updates-to-existing-functions and docs-for-new-functions, i.e. using @since as some sort of flag. Existing functions could have existing arguments changed, deprecated, or new arguments added, which would not be reflected in the function's @since.

See my point above. I think developers are quick enough to understand that the x.x+1 docs they're viewing are for a future release, not to be confused with docs for the current stable.

Maybe this aspect is worth a ticket of its own so it can be debated since it has arisen as tangential discussions for at least two tickets.

Probably.

#7 @atimmer
5 years ago

  • Cc timmermansanton@… added

#8 @coffee2code
5 years ago

  • Owner set to coffee2code
  • Resolution set to fixed
  • Status changed from new to closed

In 639:

Code Reference: Add link to core.trac to view code source for item. Fixes #447. props DrewAPicture

  • Change 'Source:' label to 'Source file:'
  • Add 'View source...' link that points to relevant line in core.trac when appropriate
Note: See TracTickets for help on using tickets.