JSHint support #65

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

XhmikosR commented Oct 30, 2012

No description provided.

lib/markdown.js
@@ -535,12 +537,12 @@ Markdown.dialects.Gruber = {
// We have to grab all lines for a li and call processInline on them
// once as there are some inline things that can span lines.
var li_accumulate = "";
+ var nl = "",
+ l = lines[line_no].replace(/^\n/, function(n) { nl = n; return ""; });
@evilstreak

evilstreak Apr 21, 2013

Owner

This change will affect the behaviour, since it's no longer resetting nl and calculating l at the start of each loop.

lib/markdown.js
@@ -1181,6 +1184,7 @@ function split_meta_hash( meta_string ) {
// shift off the next letter to be used straight away.
// it was escaped so we'll keep it whatever it is
letter = meta.shift();
+ break;
@evilstreak

evilstreak Apr 21, 2013

Owner

This changes the behaviour as it would previously flow over into the default and add the letter onto the parts.

lib/markdown.js
@@ -3,6 +3,8 @@
// Copyright (c) 2009-2010 Ash Berlin
// Copyright (c) 2011 Christoph Dorn <christoph@christophdorn.com> (http://www.christophdorn.com)
+/*jshint forin:true, noarg:true, noempty:true, eqeqeq:true, bitwise:true, undef:true, unused:true, browser:true, devel:true, jquery:true, maxerr:100 */
@evilstreak

evilstreak Apr 21, 2013

Owner

I think browser, devel and jquery should all be false, as this library is expected to work in environments like Node.

lib/markdown.js
- : li;
+ var add_to = li[li.length -1] instanceof Array && li[li.length - 1][0] === "para" ?
+ li[li.length -1] :
+ li;
@evilstreak

evilstreak Apr 21, 2013

Owner

I'm not convinced this change is for the better. Is there an explanation for preferring it? If not, is there a flag to permit it? The same comment applies to similar changes below.

Owner

evilstreak commented Apr 21, 2013

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.

Contributor

XhmikosR commented Apr 22, 2013

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

Contributor

XhmikosR commented Apr 22, 2013

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.

Owner

evilstreak commented Apr 22, 2013

@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?

Contributor

XhmikosR commented Apr 22, 2013

@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.

Owner

evilstreak commented Apr 22, 2013

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.

Contributor

XhmikosR commented Apr 22, 2013

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.

Contributor

XhmikosR commented Apr 26, 2013

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.

test/regressions.t.js
@@ -482,7 +482,7 @@ test( "inline_link", function(t, md) {
[ [ "link", { href: "url", title: "tit'le" }, "text" ], " after')" ],
"inline link III" );
- t.equivalent( md.processInline( "[text](url \"title\")" ),
+ t.equivalent( md.processInline( "[text](url 'title')" ),
@ashb

ashb Apr 29, 2013

Collaborator

This change and the one one line 454 is wrong.

If you desperately don't want \" then change the whole thing to single quotes, not the double quote in the string to single quotes.

@XhmikosR

XhmikosR Apr 29, 2013

Contributor

I don't see it wrong. It gets rid of the unnecessary escape.

@ashb

ashb Apr 29, 2013

Collaborator

You are changing the input to a test from (url "title") to (url 'title') - this is no longer testing the same thing.

@XhmikosR

XhmikosR Apr 29, 2013

Contributor

OK then, I'll revert this part when I manage to rebase against master.

Collaborator

ashb commented Apr 29, 2013

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

Collaborator

ashb commented Apr 29, 2013

@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!

Contributor

XhmikosR commented Apr 29, 2013

@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.

Contributor

XhmikosR commented Jun 1, 2013

PR updated.

Contributor

XhmikosR commented Jul 27, 2013

Bump

Collaborator

ashb commented Jul 27, 2013

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?

Contributor

XhmikosR commented Jul 27, 2013

@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.

Contributor

XhmikosR commented Jul 28, 2013

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

Contributor

XhmikosR commented Aug 16, 2013

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

Collaborator

ashb commented Aug 16, 2013

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.

Contributor

XhmikosR commented Aug 16, 2013

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.

@ghost ghost assigned ashb Aug 22, 2013

@ashb ashb closed this in 866e281 Aug 22, 2013

Collaborator

ashb commented Aug 22, 2013

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

Contributor

XhmikosR commented Aug 22, 2013

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment