sul-cidr / noh

Noh as Intermedia
http://noh.stanford.edu/
MIT License
4 stars 2 forks source link

Lock down prettier #556

Closed simonwiles closed 4 years ago

simonwiles commented 4 years ago

The logic here was to get things ready for Jarek to use prettier-atom on his local set up. It ended up being a bit of a rabbit-hole. Standardising prettier config for html/md files seems sensible, even at this late stage.

Having configured prettier appropriately (including the necessity for a version bump to ensure appropriate parsers are available, and that it can handle liquid tags and so on), I've run it over the entire codebase. Note too that I updated the lint-staged command so that husky will ensure it's run over all staged files before their committed going forwards.

Note, @inefable in particular, that the results are not exactly the same as your branch -- you must have some different settings in your VS Code set up. However, checking out this code should enforce these settings for everyone gong forwards (both in editors and in pre-commit linting). Some of the prettier rules are not to my personal taste, but, especially in a multi-author project, consistency trumps individual preference, and prettie'rs strength is its unapologetically opinionated nature :)

About 40 files in total required fixes to broken html to be parsed properly (mostly missing closing tags, but not always). I did have to exclude one file from being touched by prettier altogether.

Discussion is welcome, but I would like to be able to check this out on Jarek's machine tomorrow afternoon if possible (sorry!).

simonwiles commented 4 years ago

Minor question: is src/_includes/filters.html ignored because it's almost all Liquid directives?

Prettier chokes pretty hard on things like this: image

See comment on 4048a9e.

simonwiles commented 4 years ago

Re: the footnotes problem:

So I think I've arrived at a solution. Things to note:

  • markdown (*.md) files, because they contain so much HTML (indeed, for the most part, they're more HTML files with the occasional sprinkling of markdown) must be parsed by Prettier as HTML -- as a result, Prettier's documented <!-- prettier-ignore-start --> and <!-- prettier-ignore-end --> are not available.
  • the HTML parser (kind-of) respects <!-- prettier-ignore -->, but only at the element level. What this means for us is that "loose" bits of markdown (i.e. footnotes) get wrapped and break, even when preceeded by <!-- prettier-ignore -->. This could be dealt with by placing the ignore pragma before the main div, but only at the expense of (not) formatting everything inside.

  • the solution is to place the footnotes themselves inside the placeholder, which already requires the pragma anyway. This has a certain neatness to it anyway. The wiki will need to be updated to reflect this.

simonwiles commented 4 years ago

Some notes for VS Code users (I think that's three of us?):

For the record, Jarek is set up with prettier-atom in a way that will respect all of this, so he's good to go.

For the future, @broadwell , we might consider committing .vscode folders to github for projects like this.

broadwell commented 4 years ago

The fix for the footnotes looks good! And placing the footnotes content in the placeholder div arguably is how we should have been doing it all along.

One minor wrinkle is that the site home page as currently written also has a footnote! So I've opened a PR against this one that addresses it, as well as a missing space in the text that has been bothering me for months. It's also highly likely however that @inefable's planned overhaul of the front page will do away with the footnote anyway.

simonwiles commented 4 years ago

@broadwell rather than rebase your branch on this and then merge it, I've instead cherry-picked your commit onto this branch, as it was a lot simpler. Hope that's okay :)

@broadwell, @inefable, this is ready to be checked, please -- in particular, please check the work you did with #554 and #557 respectively, and make sure I didn't mung anything while resolving merge conflict by hand. Also, please be aware of my notes above regarding VS Code -- suggestions or alternatives are totally on the table if you're not keen on the way I've gone about this part :)

simonwiles commented 4 years ago

Good catch, Peter -- I was going to blame it on prettier stripping an attribute without a value, but it seems it works fine, so I must just have missed it when hand-reconciling merge conflicts. Fixed and rebased (but force-pushed, so your review has been invalidated, sorry -- no need to do it again).

inefable commented 4 years ago

@broadwell Looks good to me too!