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

Update manifest generation to use relative urls #14739

Closed
wants to merge 2 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Apr 1, 2019

NOT TO MERGE UNTIL CORE CHANGES ARE ADDRESSED

Fixes #14303
Related core ticket: https://meta.trac.wordpress.org/ticket/4244

@oandregal oandregal changed the title Update/manifest relative urls Update manifest generation to use relative urls Apr 1, 2019
@@ -2,1297 +2,1297 @@
{
"title": "Gutenberg Handbook",
"slug": "handbook",
"markdown_source": "https://raw.githubusercontent.com/WordPress/gutenberg/master/docs/readme.md",
"markdown_source": "../docs/readme.md",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking if this should be just docs/readme.md (relative to the root of the repository instead of being relative to the file). I wonder which one is simpler for the importer. cc @dd32

Copy link
Member

@dd32 dd32 Apr 2, 2019

Choose a reason for hiding this comment

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

The docs importer only accepts an absolute URI here. https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/themes/pub/gutenberg/inc/docs-importer.php#L25

Adding the functionality to support a relative URL is doable if needed.

@oandregal
Copy link
Member Author

Note that there is a related issue to migrate contents to DevHub https://meta.trac.wordpress.org/ticket/4388

@youknowriad
Copy link
Contributor

@nosolosw Do you think we can provide the meta patch in parallel to the work done here? that way we merge both at the same time?

@youknowriad youknowriad mentioned this pull request May 6, 2019
4 tasks
@oandregal oandregal force-pushed the update/manifest-relative-urls branch from e958b88 to 5798ecb Compare May 8, 2019 08:53
@oandregal
Copy link
Member Author

Rebased from master, as there were notable and conflicting changes to the docs tool lately,

@coffee2code
Copy link

coffee2code commented May 10, 2019

Can I get some clarification on how relative paths were envisioned to work? Currently, two manifests are utilized (the second via #15254).

manifest.json has:

manifest-devhub.json has:

In both cases, it appears .. represents https://raw.githubusercontent.com/WordPress/gutenberg/master, assuming both manifests are meant to point to the same file (as they did before this change).

My question: for manifest-devhub.json, should the relative path resolve to:

  1. https://raw.githubusercontent.com/WordPress/gutenberg/master/docs/designers-developers/key-concepts.md

or should it resolve to:

  1. https://raw.githubusercontent.com/WordPress/gutenberg/add/devhub-manifest/docs/designers-developers/key-concepts.md

Bear in mind, the path that relative paths are relative to is the location for the manifest. So my belief is that (2) is the intent. This allows for a manifest in a branch to easily reference files relative to its own branch. The markdown_source values of designers-developers/key-concepts.md, /designers-developers/key-concepts.md, and ../docs/designers-developers/key-concepts.md would all be equivalent and resolve to (2). (I have this implemented locally.)

However, if you expect the path to be relative to the repo root, then I believe you'd have to use a markdown_source value like master/docs/designers-developers/key-concepts.md to refer to the master branch's version of a file, or something like add/devhub-manifest/docs/designers-developers/key-concepts.md to refer to the version of that file in a branch/PR.

(I'm running under the assumption that these or future manifests may want to refer to non-master versions of files.)

@oandregal
Copy link
Member Author

Bear in mind, the path that relative paths are relative to is the location for the manifest. So my belief is that (2) is the intent. This allows for a manifest in a branch to easily reference files relative to its own branch.

Yep. The idea is to be able to pinpoint the handbook content to a specific commit hash (branch, tag, etc) - see conversation.

@coffee2code
Copy link

So then as far as my question goes, the relative link of ../docs/designers-developers/key-concepts.md currently found in manifest-devhub.json should expand to refer to https://raw.githubusercontent.com/WordPress/gutenberg/add/devhub-manifest/docs/designers-developers/key-concepts.md and not master, correct?

@coffee2code
Copy link

Ah, I see the answer to my question in a comment on #15254, and that my presumption of the relative link pointing to the manifest's branch is correct.

@oandregal
Copy link
Member Author

This can be closed. It is superseded by #15254 which includes relative paths support.

Current handbook sync has been disabled and relative paths not implemented for this. See https://meta.trac.wordpress.org/ticket/4244#comment:7

@oandregal oandregal closed this May 14, 2019
@oandregal oandregal deleted the update/manifest-relative-urls branch May 14, 2019 07:45
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.

Update the documentation manifest to use relative links instead of pointing to "master" explicitely.
4 participants