Skip to content

Fix Testing/Linting Issues #1112

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

Merged
merged 6 commits into from
Apr 18, 2017
Merged

Fix Testing/Linting Issues #1112

merged 6 commits into from
Apr 18, 2017

Conversation

skipjack
Copy link
Collaborator

@skipjack skipjack commented Apr 15, 2017

Fixes #1041

This is an attempt to resolve all of our linting and testing issues. It contains:

  • Removal of eslint script for markdown code blocks
  • Fixes for a whole slew of markdownlint errors
  • Minor updates to .markdownlintrc to loosen rules

Take aways...

  • We could add eslint-ing for markdown code blocks if someone takes the time to write a separate eslintrc for that. I'm not sure how much value it really adds though.
  • Certain patches I made on the plugin markdown files should be carried over to their respective readmes in per-repo PRs. Or we should figure out how to exclude the auto-fetched files from the linting process.

In general, I think we should try to be a little more careful about adding scripts or processes before they're fully ready. Also, for the ones that we do want to keep, these should actually fail hard in the travis build so we pick up on things like markdown or prose issues instead of letting them build up.

The last one that needs fixing is lint:social. It only emits warnings right now but for some reason it's still failing hard. @bebraw @TheLarkInn any ideas? (I forget who implemented that one)

Once we get lint:social either removed or resolved, I'll rebase to resolve the conflicts and this should be good to go.

@simon04
Copy link
Collaborator

simon04 commented Apr 15, 2017

Great work. Once all issues are resolved, those tests also should be added to CI.

Running alex *.md content/ brings up the following 50 warnings: https://gist.github.com/simon04/b823cd424f1f3645faa7592e5f52f91c

$ node_modules/.bin/alex --version    
3.1.0

@bebraw
Copy link
Contributor

bebraw commented Apr 15, 2017

The last one that needs fixing is lint:social. It only emits warnings right now but for some reason it's still failing hard. @bebraw @TheLarkInn any ideas? (I forget who implemented that one)

@wooorm Any idea on this alex warning issue?

@wooorm
Copy link

wooorm commented Apr 15, 2017

@wooorm Any idea on this alex warning issue?

A “warning” in alex’s sense is enough to fail the tests I think. You could go with alex --diff to only check the changed lines in Travis though?

@skipjack
Copy link
Collaborator Author

@webpack/documentation-team idk after taking a look at the warnings I saw locally and the ones @simon04 shared, I'd say the way it's configured now is way too tight for our use case. The fact that it's flagging things like disabled, executed and bigger makes it seem like it's going to get in our way more than help us.

Has anyone found it useful so far? Or is anyone familiar enough with it to re-configure it in a looser way? Either way, imho I think we should remove it for now and re-integrate more thoroughly later on if anything. I'd like to get this merged and back into the CI process sooner rather than later so I don't have to go through fixing a bunch more errors after the next round of PRs (would be nice if markdownlint had a fix option like eslint 😁).

@wooorm
Copy link

wooorm commented Apr 17, 2017

Note that you could also add an .alexrc file, which turns off these warnings, like so:

{
  "allow": [
    "bigger",
    "color",
    "lies",
    "execute",
    "executed",
    "execution",
    "host-hostess",
    "disabled",
    "crash",
    "failure",
    "period",
    "hook",
    "dirty",
    "host-hostess",
    "fire",
    "remains",
    "jade",
    "failed"
  ]
}

Or, alternatively, run alex as previously mentioned as alex --diff.
Or finally, I personally use alex on my computer only, once in a while, to check markdown files — not in a CI.

@skipjack
Copy link
Collaborator Author

@wooorm ah ok... I'll try that and then we can continue to fiddle around this more down the road (or just keep adding words to the allowed list.

@skipjack
Copy link
Collaborator Author

skipjack commented Apr 17, 2017

@wooorm that solution works, thanks.

@webpack/documentation-team just need to test proselint and we should be good to go.

Update: Surprisingly proselint throws no errors 🎉 , so I'm going to rebase and then request some reviewers.

The `lint:md` command isn't very semantic as it stands right now and
also isn't very useful. We'd have to split out a separate eslint config for
this and even then it might be hard to strike a nice balance.
@skipjack skipjack force-pushed the fix-test-and-lint-scripts branch from 6614d3c to d0e0a49 Compare April 17, 2017 21:32
@skipjack skipjack requested review from simon04 and bebraw April 17, 2017 21:32
@skipjack
Copy link
Collaborator Author

skipjack commented Apr 17, 2017

Ok rebased and resolved conflicts...

Once all issues are resolved, those tests also should be added to CI.

Definitely agree... is this as simple as adding a line to the travis.yml? If someone points me in the right direction we can knock this out now as well.

@bebraw
Copy link
Contributor

bebraw commented Apr 17, 2017

Yeah, tweak travis.yml so it picks up the tests you want.

@skipjack
Copy link
Collaborator Author

This should be good to go 👍 , all tests ran and the CI build passed.

@bebraw bebraw merged commit ab2a604 into master Apr 18, 2017
@skipjack skipjack deleted the fix-test-and-lint-scripts branch April 20, 2017 04:39
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.

npm run test => error
4 participants