JSHint support

[evilstreak]

In general, the changes look good and I'm in favour of them. There's a few I have issue with, most notably those that change the behaviour of the code!

If you have the time and inclination to look at this in the next week that'd be great, otherwise I'll make the suggested modifications when I get a chance and merge it in.

[XhmikosR]

Hi. I'll improve the PR soon and cc you.

[XhmikosR]

cc @evilstreak.

I added JSHint support properly. I also reverted the changes you commented.

Please check .jshintrc and let me know if you want me to change anything.

[evilstreak]

@XhmikosR Thanks for the update.

When I check out your branch and run npm run-script lint I get 111 errors, mostly about indentation. Is that what you expect?

I only have one comment about .jshintrc, which is that when I said the library is expected to work in environments like Node, lib/markdown.js is also expected to work in browsers. Can you set node: false just for that file?

[XhmikosR]

@evilstreak: I have changed that just before you try it. Please check out the branch again.

Personally, I'd like to have "curly": true, "indent": 2 but it's your call.

EDIT: Now the PR should be final.

[evilstreak]

This pull request is starting to get a little busy!

Tap 0.4.1 behaves oddly (no output by default, doesn't seem to run all the test files, and incredibly slow to exit after running tests) compared to the previous version I had installed (0.3.3 behaves as I'm used to). I'd like to investigate this further before updating the dependency.

The changes to the README are in general fine, though the list item indentation change doesn't seem right (it's gone from 4-space indent to a 5-space indent).

The changes to both lib/markdown.js and the tests to improve consistency and strictness seem good, though I think the jshint options in lib/markdown.js are still wrong — the code in that file should work whether it's being run in the browser or in node, so jshint should assume global variables from neither environment (i.e. browser: false, node: false, unless I've misunderstood those flags).

Finally, I'm hesitant to add an npm run-script lint task that is producing a lot of warnings out of the box.

In conclusion: I'd prefer multiple smaller (more focused!) pull requests over one large one. I think several of your changes are immediately pullable, or very close to pullable, and some are further off.

I'd accept a pull request right now that consisted solely of the code improvement changes, without the changes to package.json and README.md. For the other three changes (updating dependencies, updating the README, and adding a lint task), I'd prefer to see new pull requests so we can get them smoothed out before pulling.

[XhmikosR]

For tap, we can specify another version, but it's better to follow semver than using 0, or 1.

For the Readme, no indentation is needed for the lists; I'd personally remove the indentation.

We have browser: false in .jshintrc but for markdown.js browser is true. I think it's fine.

The lint task is one with the code changes. Without it you have to do it randomly. The warnings are not so many, and with a few more tweaking they can be reduced more.

I split the changes. The PR's are dependent so I will need to rebase if one is merged before merging the next one.

[XhmikosR]

I added a couple more commits. I'm not sure if this is the style you prefer, but now it's consistent by using brackets.

Let me know your thoughts.

[ashb]

I disagree with the re-indentation in 7ad51bdd9027e02267734745 - indenting the entire file for the (function( expose ) { is pointless in my view.

[ashb]

@XhmikosR could you rebase this pull request against master - I'm not quite sure which (if any?) parts of this have been included in other pull requests etc.

Thanks!

[XhmikosR]

@ashb: now it should be ok. The indentation patch is now much smaller since I only changed the part that used 4 spaces instead of 2 like most of the code does.

[XhmikosR]

PR updated.

[XhmikosR]

Bump

[ashb]

Apologies for letting this lie fallow for so long - we've been rather distracted with other things :(

According to travis this branch is failing its tests while master is currently passing https://travis-ci.org/evilstreak/markdown-js/builds/9542984. The error rather confusing:

Cannot call method 'replace' of undefined at /home/travis/build/evilstreak/markdown-js/lib/markdown.js line 393

We're we waiting on @evilstreak's (browser) test refactor for this or was that something else?

[XhmikosR]

@ashb: No worries, I understand you both are busy people.

I noticed that the tests are failing, but since @evilstreak suggested to use tap @0.3.3 I cannot test things on Windows; it always failed for me. I need to use tap 0.4.3 to actually see the failed tests.

EDIT:

I can't see something wrong with my changes, but I'm missing something probably. Or it's the strict comparisons used.

[XhmikosR]

The issue with the failing tests seems to come from this. I removed for now until someone else has a look.

EDIT: Travis still fails and the JSHint warnings must be related.

> markdown@0.5.0 check C:\Users\xmr\Desktop\markdown-js
> jshint .

lib\markdown.js: line 265, col 5, 'blocks' is already defined.
lib\markdown.js: line 265, col 13, 'blocks' is a statement label.
lib\markdown.js: line 266, col 34, 'blocks' is a statement label.
lib\markdown.js: line 266, col 50, 'blocks' is a statement label.
lib\markdown.js: line 379, col 76, Don't make functions within a loop.
lib\markdown.js: line 578, col 85, Don't make functions within a loop.
lib\markdown.js: line 652, col 71, 'nl' used out of scope.
lib\markdown.js: line 824, col 14, 'res' is already defined.
lib\markdown.js: line 927, col 15, 'link' is already defined.
lib\markdown.js: line 1109, col 32, ['__'] is better written in dot notation.
lib\markdown.js: line 1111, col 32, ['_'] is better written in dot notation.
lib\markdown.js: line 1117, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1130, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1249, col 30, Expected a 'break' statement before 'default'.
lib\markdown.js: line 1276, col 9, Creating global 'for' variable. Should be 'for (var p ...'.
lib\markdown.js: line 1276, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1317, col 11, Creating global 'for' variable. Should be 'for (var a ...'.
lib\markdown.js: line 1317, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1337, col 9, Creating global 'for' variable. Should be 'for (var a ...'.
lib\markdown.js: line 1337, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1359, col 13, 'm' is already defined.
lib\markdown.js: line 1393, col 29, Expected a conditional expression and instead saw an assignment.
lib\markdown.js: line 1405, col 38, Expected a conditional expression and instead saw an assignment.
lib\markdown.js: line 1413, col 15, 'table' is already defined.
lib\markdown.js: line 1482, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1599, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1627, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 1710, col 15, 'ref' is already defined.
lib\markdown.js: line 1738, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
lib\markdown.js: line 122, col 11, 'uneval' is not defined.
lib\markdown.js: line 124, col 11, 'uneval' is not defined.
lib\markdown.js: line 126, col 11, 'uneval' is not defined.
lib\markdown.js: line 1277, col 20, 'p' is not defined.
lib\markdown.js: line 1318, col 13, 'a' is not defined.
lib\markdown.js: line 1318, col 25, 'a' is not defined.
lib\markdown.js: line 1338, col 11, 'a' is not defined.
lib\markdown.js: line 1338, col 23, 'a' is not defined.

test\features.t.js: line 40, col 11, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
test\features.t.js: line 54, col 9, Don't make functions within a loop.
test\features.t.js: line 26, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
test\features.t.js: line 82, col 1, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
test\features.t.js: line 44, col 17, 'text' is not defined.
test\features.t.js: line 49, col 47, 'text' is not defined.

test\regressions.t.js: line 48, col 5, 'h2' is not defined.

44 errors

[XhmikosR]

bump No 2...

I haven't fixed the Travis failures; I'd rather one of you @evilstreak @ashb have a look.

I'm pretty sure they are caused because of the undefined variables like blocks, etc.

As you can see JSHint isn't just for missing semicolons :P

[ashb]

God we're awful at this open source development lark. Sorry.

Good news is that evilstrak and I are planning on spending some time hacking next week - this will be one of the first things we look at.

[XhmikosR]

OK, good to hear that. Let me know if you need any changes in the PR.

I decided to use JSHint's feature to load its config from package.json instead of using a separate file.

[ashb]

Thanks - I didn't use most of the commits directly - a few commits I cherry picked, a few I just did differently.

I've also added you to the contributors section in the package.json

[XhmikosR]

Thanks for looking into the PR.

The thing with your way is that I dont show as the patch author...

You could have told me if you wanted me to make any changes, but anyway I'm glad the PR is merged.