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

Add Vale to Travis CI #147

Merged
merged 7 commits into from Mar 11, 2017
Merged

Add Vale to Travis CI #147

merged 7 commits into from Mar 11, 2017

Conversation

jdkato
Copy link
Contributor

@jdkato jdkato commented Feb 16, 2017

Hi,

This PR adds Vale to your existing Travis CI setup (and replaces test-branding.py). I made a post about Vale on the WTD forum.

I don't necessarily expect this to be merged; I just thought you might be interested in seeing how it could be used (it's also relevant to #58).


In terms of the actual linting, I implemented some of the ideas discussed in the December 2016 newsletter:

  • A suggestion about sentences longer than 28 words;
  • a warning about unnecessary wordiness;
  • a warning about deviating from plain language;
  • an error about inconsistent use of "e.g." and "i.e."; and
  • an error about using "Write The Docs" or "Read The Docs."

The build will only fail on errors (it's currently failing over the use of "Read The Docs" in origin-story.rst and a number of different forms of "e.g." throughout the docs). Obviously, you may want to tweak the individual checks yourselves.

Thanks for taking a look!

@ericholscher
Copy link
Member

This is super neat and interesting! Thanks for taking the time to set it up. I'm definitely interested in adopting it, and will go through and try to fix the errors :)

@plaindocs
Copy link
Contributor

@ericholscher leave the fixup to me if you like.

@ericholscher
Copy link
Member

Works for me! :)

@plaindocs
Copy link
Contributor

OK, I'm picking this up, a few days late. @jdkato many thanks for this work.

A couple of thoughts:

  • I'll fix the actual errors reported by vale, because why not.

  • If we're going to merge I'd like to discuss some of the following

    • How easy is it to implement checks for gendered or insulting language, for example

    • Pros and cons vs either the remark family or an overarching linter like coala ci which I like for the simplicity.

    • what checks we want to implement on WTD

I'd have some preference for running only error linters on a default build, not warnings, to reduce noise, and asking people to run those locally but I doubt that is actually practical, and definitely outside the scope of this PR for now.

plaindocs added a commit that referenced this pull request Feb 25, 2017
@plaindocs
Copy link
Contributor

mmmm, not quite sure why the build now can't find vale. Running go get github.com/jdkato/vale locally also does not work.

@jdkato if you cherrypick that commit into your branch it should fix the 9 errors that the build flagged

@jdkato
Copy link
Contributor Author

jdkato commented Feb 25, 2017

Hi @plaindocs,

Sorry, I've recently restructured the project so that it works as a Go library in addition to a CLI tool. This changes the go get command slightly. I'll update this and reply to your previous comment tomorrow.

@jdkato
Copy link
Contributor Author

jdkato commented Feb 25, 2017

I've fixed the go get issue and cherrypicked 848826c. The build is now passing on my fork, but it looks like eafc403 introduced another "e.g." error (code-of-conduct-reporting.rst, line 20).


With regard to your previous comment,

How easy is it to implement checks for gendered or insulting language, for example

Creating a new check (or "rule" in Vale terminology) is simply a matter of writing a YAML file that extends one of Vale's built-in checks and adding it to a style (e.g., the WTD style I made for this PR). For example,

# GenderBias.yml
type: substitution # the type of check we're extending.
message: Consider using '%s' instead of '%s'
# ^ What we display to the user.
ignorecase: true
level: error # the severity level
map:
  man enough: strong enough
  mankind: human kind

In this case, we would flag any occurrences of "man enough" or "mankind." An example message would be:

line:column:<style name>.GenderBias: Consider using 'human kind' instead of 'mankind'

(You can see a more thorough example here.)

Pros and cons vs either the remark family or an overarching linter like coala ci which I like for the simplicity.

  • retext has a number of strengths: it's easier to integrate with a web application since it's written in JavaScript, it has strong support for Markdown and HTML (RedPen and textlint also offer good markup support), and it has a better plugin system from a programming standpoint.

  • coala ci, from what I understand, is something that you'd use with Vale (or another prose linter) rather than in place of it. For instance, you might have the PEP8Bear lint Python source while a ValeBear lints your project's prose (the WriteGoodLintBear is an example of this).

I'd summarize Vale's pros and cons as follows:

  1. It's much newer than any of the alternatives (weeks vs. years, in fact), which naturally means that it's likely to have more details that need to ironed out.

  2. My primary reason for starting the project was to make it easy to extend. retext is extended through node packages (e.g., here's an inconsiderate language extension for retext), which I think is often more work than it needs to be.

  3. Vale supports more markup formats than retext, but is currently sensitive to its input formatting. A Markdown file without blank lines between paragraphs and codeblocks, for example, won't be linted correctly. This is definitely something I'm going to change, but I've found that it is rarely an issue in practice (it seemed to have reasonably handled all ~800 markup documents in this repo).

what checks we want to implement on WTD

If you'd like me to make a more thorough example WTD style for you to compare against other linters, just let me know. I'd gladly use the opportunity to test the extension system.

I'd have some preference for running only error linters on a default build, not warnings, to reduce noise, and asking people to run those locally but I doubt that is actually practical, and definitely outside the scope of this PR for now.

This can be set via the MinAlertLevel setting in a .vale file ("suggestion" is the default but it can also be "warning" or "error").

@plaindocs plaindocs mentioned this pull request Feb 26, 2017
@plaindocs
Copy link
Contributor

Thanks for the detailed reply @jdkato !

My concern here is that I don't want to hugely increase friction to additions / edits to the docs by failing the build on (imho) trivial things like e.g., vs e.g..

I'd like a small subset of important things to fail the build, such as discriminatory language, the RTD/WTD branding issue. But that list is open for discussion, and definitely looks like it should be easily doable with vale.

I think the important thing generally is an easy to implement linter with sensible defaults so that people can drop in a linter that checks for BAD THINGS (TM) but does not enforce a style in an Economost / Chicago Manual of Style type way - so people can select that themselves.

And I think that kind of sensible defaults is something I'd be happy to help with, and would be very useful for many documentarians around these parts.

So, I think the way forward is to start with a small list of checks, merge this in once we have them and build on that.

Can we brainstorm a list of checks we want to fail the build in a separate issue #157 , and I'll take care of implementing them into vale yaml, with help from @jdkato .

@ericholscher
Copy link
Member

This looks good to me as is. I'll let @plaindocs merge it since he has more context, but I'd like to get this in, since it is a bit more robust than the current version :)

@plaindocs
Copy link
Contributor

I'm working on this, and will try to merge it in asap.

Current issues, hoping @jdkato can help:

  • How to exclude a directory docs/_build ? This probably does not matter for the CI, but I'd expect it to be a common feature request.

  • How to include specific checks from a style we're not based off without copying the files?

    BasedOnStyles = WTD
    vale.Redundancy = YES # Core style, works
    TheEconomist.Hectoring = YES # Does not unless you copy the file your style dir
    
  • some issues parsing html in md:

    docs/conf/eu/2015/speakers.md
    251:3  error  '</div>' is repeated!  vale.Repetition 
    
  • Re ^ is there a way to exclude specific checks from specific files so we can move ahead and merge without that causing a build fail?

  • I'm guessing you can't override the importance of a check without editing it? Ie, include check X from the Economist but treat it as error not warning?

  • I'm not sure the Branding test is actually running. Could you have changed something with the reorg over the last few days? I don't think I broke it - but that is also possible.

@plaindocs
Copy link
Contributor

Some config changes: 22541ce

@jdkato
Copy link
Contributor Author

jdkato commented Mar 5, 2017

How to exclude a directory docs/_build ? This probably does not matter for the CI, but I'd expect it to be a common feature request.

You can currently only specify what you want to lint using the --glob flag (or in your config file), but I agree that this should be supported.

How to include specific checks from a style we're not based off without copying the files?

Vale only includes the rules in the vale style—so you can't reference another style without it being on your StylesPath. TheEconomist is simply an example extension (similar to what WTD is).

I'm guessing you can't override the importance of a check without editing it? Ie, include check X from the Economist but treat it as error not warning?

Not yet ... but it's on my TODO list (errata-ai/vale#18).

I'm not sure the Branding test is actually running. Could you have changed something with the reorg over the last few days? I don't think I broke it - but that is also possible.

I'll take a look.

some issues parsing html in md:

Yeah, to be honest, I overestimated Vale's robustness (sorry!) at handling markup when I opened this PR. You may want to wait for the resolution of errata-ai/vale#19 before moving forward. I made progress on it this weekend, but it's still not ready.

@plaindocs
Copy link
Contributor

Thanks for the answers!

I'll see if I can tweak the config docs to make the Style guide thing more clear.

Yeah, I guess we'll hold off a little and see how things go. I've tracked both those issues.

If we can implement an exclude file/directory mechanism then we could probably merge this in even if the html parsing isn't perfect - by ignoring the file with the issue.

@jdkato
Copy link
Contributor Author

jdkato commented Mar 6, 2017

I'm not sure the Branding test is actually running. Could you have changed something with the reorg over the last few days? I don't think I broke it - but that is also possible.

@plaindocs,

I checked this out and it was my fault: errata-ai/vale@0fe6389 split up the check extension points and changed some of the field names, but I forgot to change "type" to "extends" when I updated my fork in https://github.com/jdkato/www/commit/abf28106ac225bc7c4f662ed9297612621b929ab.

I fixed my fork and I'm also going to work on adding better error reporting when Vale fails to load rule.

If we can implement an exclude file/directory mechanism then we could probably merge this in even if the html parsing isn't perfect - by ignoring the file with the issue.

I was thinking about just adding a --ignore flag for now that accepts a glob pattern (ultimately, it might be nice to respect .gitignore files).

@plaindocs
Copy link
Contributor

--ignore flag sounds great.

@plaindocs
Copy link
Contributor

Cheers for the fixup. :-) I think as soon as that --ignore glob is working I'll re test and merge what we have.

@jdkato
Copy link
Contributor Author

jdkato commented Mar 9, 2017

I should have the --ignore flag implemented within a day or so.

I've also improved markup parsing (I discussed the technique at errata-ai/vale#19). I'm sure it's stil not perfect, but the issues with embedded HTML appear to be fixed.

Since Vale now uses rst2html to process reStructuredText, it needs
access to docutils (which is a dependency of Sphinx).
@jdkato
Copy link
Contributor Author

jdkato commented Mar 10, 2017

Instead of duplicating the functionality of --glob under the name --ignore, I've added support for bash-style extglob syntax:

# The "!" negates the glob, so this will ignore the `_build` directory
$ vale --glob='!*/_build/*' docs

@plaindocs plaindocs mentioned this pull request Mar 11, 2017
@plaindocs plaindocs merged commit 39ffd7b into writethedocs:master Mar 11, 2017
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.

None yet

3 participants