adlnet / xAPI-Spec

The xAPI Specification describes communication about learner activity and experiences between technologies.
https://adlnet.gov/projects/xapi/
905 stars 405 forks source link

Added anchor tags to all headings, table rows, and bullet points #944

Closed stevenvergenz closed 8 years ago

stevenvergenz commented 8 years ago

This enables users to hyperlink-reference individual subsections using the same naming scheme that the human-readable portions use. I.e. <a href="./xAPI-Data.md#2.4.4">Statement property: Object</a>.

You can also now reference particular items in tables and lists. I.e. <a href="./xAPI-Data.md#2.4.8.s3.b2">The stored property MUST be set by the LRS</a>.

These anchor tags were auto-generated by a Node.js script, which for repository hygiene I've removed, but is accessible at https://github.com/adlnet/xAPI-Spec/blob/61488b37ea21280bf74a6f1a46a09ede8f818704/tagger.js

garemoko commented 8 years ago

This looks like a heck of a lot of work so I feel bad for saying this, but named links are MUCH better than automatically generated number links because people link to the document externally a lot and sections do get changed.

Would it be possible to just use the auto generated links for places that don't already have a named link, or even better, find a way to retain the named links and add to them intelligently e.g. something like ./xAPI-Communication.md#content-types-table1

The other problem with this PR is that it's some substantial that Github can't render the diff, making it very hard to review. Is there any way we can break this up into smaller PRs, or perhaps review commit by commit?

stevenvergenz commented 8 years ago

This PR does not remove any existing tags, so the various existing links should all still work. I merely updated the TOC for simplicity.

As far as the diff goes, since all the tags were added in one pass of the script, it's not really feasible to show incremental changes. However, I could commit the diffs as a new file to be viewed?

stevenvergenz commented 8 years ago

I should provide some context. We are working on the conformance testing suite, and need the ability to reference individual requirements in the spec from the various tests. Using the pre-defined section headings has the benefit of being both machine- and human-readable.

stevenvergenz commented 8 years ago

This PR can be reviewed here: https://github.com/adlnet/xAPI-Spec/blob/9d5fdc44409d9c83db5cd32f073c8c14158edc4b/diff.txt

bscSCORM commented 8 years ago

I suggest we keep the links in the TOC as they were, referencing named anchors. This is because if someone is inclined to save a link into the document, they're likely to use a TOC link to do so, and it would be good if that link still pointed where intended if the numbers are changed.

The generated anchors would be better if they also included the first few words, or maybe first 10 characters of the text that follows them, which should be possible because they're generated. In this way, if sections are moved after a link is saved, the link would just be a broken link rather than one pointing to the wrong text (which could be rather confusing).

Finally, this will have to be maintained. My experience has been that mixing generated and manually written code becomes a maintenance nightmare, although this is a document I expect we'll run into some of the same issues. Since the anchors are being added via a script already, and that script will already have to be re-run after any change to the document that adds or removes a section or requirement, there shouldn't be any reason that script can't be run to generate some rendered output that is never committed. This could potentially even be triggered as a commit hook, an update is made to the spec, script runs to generate an updated markdown file, that file is either rendered into html and plunked on a webserver somewhere or automatically checked in to a different repository, then links from the test suite can go to that page rather than directly to the spec in this repository.

garemoko commented 8 years ago

@stevenvergenz thanks for such a speedy and complete response!

RE: "This PR does not remove any existing tags, so the various existing links should all still work. I merely updated the TOC for simplicity." This sounds like a great approach. I'd prefer to keep the word based links in the TOC though as I believe that's how people discover those links exist in the first place. Edit: Ben already said this.

I'll make some time to check through that diff sometime soon.

stevenvergenz commented 8 years ago

@bscSCORM: Is there a need to continuously regenerate the tags? I had thought we would merely use the script to generate the tags once, and update manually going forward. Barring any more major restructuring (like from 1.0.2 to 1.0.3), I wouldn't expect it to be too difficult.

And I have no objections to using named tags for the TOC.

stevenvergenz commented 8 years ago

This PR will address issue #941.

garemoko commented 8 years ago

+1, on trust that there's no text changes :-)

andyjohnson commented 8 years ago

Per the 7/6/16 call, all issues resolved.