Add more checks to dangerfile.js

[chris48s]

A few suggestions for additional checks we might try to add:

• If PR contains tests using assert, suggest expect syntax.
• If file contains .expectJsonTypes({ object: "literal" }), suggest .expectJson({ object: "literal" }) instead.
• If PR modifies a file in /logo, remind the submitter that it should be minified (for bonus points, maybe try to work out if it has been minified - are there any obvious checks we can do here?)
• If the PR is targeted at a branch other than master, warn reviewers.
  I realise sometimes it makes sense to target another branch e.g: node-8 but this project mostly doesn't use release banches and I think it is worth flagging to the reviewer if someone targets a PR straight at gh-pages, like this: Delete me #1552 as we would probably want to change the target.

If anyone else has thoughts on this based on points that often crop up in review, lets collect them here..

[paulmelnikow]

These all sound great!

(2) would be huge!

For (3), logo minification, we could try running the logo through svgo and seeing if the output is the same. It's possible that would be more simply written as a unit test.

[platan]

Regarding (2)
There are several approaches.

1. My first thought: add a Danger rule, which operates on a diff content and try to find if there is a problem (e.g. using a regular expression).
2. Add a Danger rule, which operates on a whole file and use some AST parser to find problem. This gives better results than the regular expression but it would be complicated to find a problem in a diff.
3. Instead of using Danger (which only informs user that something should be changed) we can use ESLint to not only check code on a developer computer but also automatically fix it (using the --fix ESLint option). Currently there is no such plugin for ESLint, but I would like to create it :-).

What do you think about this?

[chris48s]

Yeah, although I originally suggested that as a danger rule, it isn't really a good candidate for that type of check because it really requires actually parsing/interpreting the code in order to do it robustly.

Thinking about it as a linting rule (option 3) is a much better way to think about it 👍

[platan]

After #2687 we have no expectJsonTypes in our code :-)
Do we want to add any other checks? If not, can we close this issue?

[paulmelnikow]

Closing sounds good to me!