adlnet / xAPI-Spec

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

Markdown formatting and whitespace #829

Open canweriotnow opened 8 years ago

canweriotnow commented 8 years ago

When working on #828 (and its predecessors) I had some issues with whitespace... I noticed 2 things that might be worth reviewing just for ease of continued maintenance:

I'd be happy to work on this if there's interest.

canweriotnow commented 8 years ago

It might even be worth setting up CI to lint on PR submission to make sure there are no regressions... should be fairly trivial on Travis (and free!)

canweriotnow commented 8 years ago

So I ran https://github.com/mivok/markdownlint on xAPI.md - there were 5,166 issues:

https://gist.github.com/canweriotnow/f12d3939ccbbf0fba3bc

I think it would be worth addressing at least some of these, especially the ones that deal with whitespace and structure

garemoko commented 8 years ago

We'd want to ignore "Header levels should only increment by one level at a time" in most/all cases but I'm sure many of these are worth addressing.

I suggest that these changes are done as the very first commit after 1.0.3 is released and from a fresh Github account created for this purpose in an attempt to limit damage to diffs between versions and contributor reports. Alternatively, this is possibly something to do just after/before splitting the parts into separate files, which is going to wreck havoc on diffs and contrib charts anyway. They certainly shouldn't be made when there are any other open PRs due to certain conflict.

+1 to Travis (if somebody else writes it!) so long as we are selective about which of the errors we enforce. Often these markdown formatting issues have zero impact on the rendered document, but some of the time they do and get missed, so this would be a genuinely helpful thing to have.

I'm not sure how I feel about converting the tables to markdown. Personally I hate markdown table syntax, but perhaps that's just something I need to get over. I can see the consistency benefits of exorcising HTML.

canweriotnow commented 8 years ago

@garemoko Luckily, markdownlint provides structure for creating style configs that say "ignore this rule" or "enforce this rule with great prejudice."

:+1: for after 1.0.3 restructure is merged - I wouldn't want to start on such a thing until that's finalized.

Once we have a solid, well-structured and happy-linting markdown document, I'd be happy to write the travis config to do CI on PRs and commits, we can just use mdl as our "unit test" tool.

tbh, I used to hate markdown tables until I realized my problem was my editor didn't have great markdown support; most need an extra plugin to be really great on Markdown (including Emacs, Vim, Atom, Sublime, etc.)

My preference would be to wait for 1.0.3 to be finalized, and the PR roundup to be finished, and then give me like, a weekend to go through and make the linter happy, and in the process we can choose which rules to enforce. My preference is ALL OF THEM, but we need to make sure it's sane as well.

As an aside, is there a reason you're against "header levels should only increment one level at a time"?

canweriotnow commented 8 years ago

Also @garemoko -

Alternatively, this is possibly something to do just after/before splitting the parts into separate files, which is going to wreck havoc on diffs and contrib charts anyway.

Is there a timeline on this splitting? It would be easier to refactor smaller files one at a time than the current massive document.

canweriotnow commented 8 years ago

Looking at the output, I could probably resolve 60% of the issues (e.g., whitespace and indentation) with sed and awk... or maybe a small Perl script. Then it's down to structure, which is a much smaller subset of the lint errors.

garemoko commented 8 years ago

@canweriotnow I believe it's one of the later tasks prior to 1.0.3 release. If I had to guess, I'd say mid-January but there's not a specific timeline we're working on. (Other than a vague notion of "late 2015" for a 1.0.3 release which I think isn't going to happen).

canweriotnow commented 8 years ago

:+1:

garemoko commented 8 years ago

I wonder if the PR for this issue can also bundle in #790

garemoko commented 8 years ago

@canweriotnow would this also standardize the spacing between list items?

vs.

canweriotnow commented 8 years ago

@garemoko In the example you give, "standard markdown" (as if such a thing existed :smile_cat:) would interpret the former as a 2-element list and the latter as 2 single-element lists, so that's pretty tough to lint... But I could see writing a rule that prohibited single-element lists or lists separated solely by newlines without intermediary content?

garemoko commented 8 years ago

That would work.

garemoko commented 8 years ago

@canweriotnow I believe now is the time if you want to take this on, perhaps one document at a time to minimize conflicts.

canweriotnow commented 8 years ago

Oops, sorry @garemoko just saw this... I will try to get on it and do the PR's one doc at a time.

andyjohnson commented 8 years ago

This is one of the few issues I could see being done after the tech writer has a go at the spec. However, if this is done first, I'll be careful to not lose the work done.

garemoko commented 8 years ago

Propose closing this because I don't think it's going to get addressed.