Skip to content

Conversation

@brettlangdon
Copy link
Contributor

this addresses #18

I have been thinking about updating the travis setup to use grunt, that way the travis build will fail when jshint doesn't pass. Thoughts?

jshint has found a few issues with rewrite.js, think we either need fix the issues or change the jshint config to allow things we want.

Running "jshint:lib" (jshint) task

   lib/rewrite.js
     56 |  if (pattern == null && node != null) {
                       ^ Use '===' to compare with 'null'.
     56 |  if (pattern == null && node != null) {
                                       ^ Use '!==' to compare with 'null'.
     60 |  if (pattern != null && node == null) {
                       ^ Use '!==' to compare with 'null'.
     60 |  if (pattern != null && node == null) {
                                       ^ Use '===' to compare with 'null'.
     64 |  if (wildcards != null && isWildcard(pattern)) {
                         ^ Use '!==' to compare with 'null'.
     87 |      && match(wildcards, pattern.value, node.value);
               ^ Bad line breaking before '&&'.
     93 |      && match(wildcards, pattern.property, node.property);
               ^ Bad line breaking before '&&'.
    103 |      && match(wildcards, pattern.right, node.right);
               ^ Bad line breaking before '&&'.
    106 |      && match(wildcards, pattern.test, node.test)
               ^ Bad line breaking before '&&'.
    107 |      && match(wildcards, pattern.update, node.update)
               ^ Bad line breaking before '&&'.
    108 |      && match(wildcards, pattern.body, node.body);
               ^ Bad line breaking before '&&'.
    170 |      && match(wildcards, pattern.init, node.init);
               ^ Bad line breaking before '&&'.
    194 |      if (wildcards != null && isWildcard(replacement)) {
                             ^ Use '!==' to compare with 'null'.
    207 |      for (var i = 0; i < replacement.elements.length; i++) {
                          ^ 'i' is already defined.
    217 |      for (var i = 0; i < replacement.arguments.length; i++) {
                          ^ 'i' is already defined.
    223 |      for (var i = 0; i < replacement.params.length; i++) {
                          ^ 'i' is already defined.
    226 |      for (var i = 0; i < replacement.defaults.length; i++) {
                          ^ 'i' is already defined.
    233 |      for (var i = 0; i < replacement.params.length; i++) {
                          ^ 'i' is already defined.
    236 |      for (var i = 0; i < replacement.defaults.length; i++) {
                          ^ 'i' is already defined.
    250 |      for (var i = 0; i < replacement.declarations.length; i++) {
                          ^ 'i' is already defined.
    273 |      for (var i = 0; i < replacement.properties.length; i++) {
                          ^ 'i' is already defined.
    310 |}
          ^ Missing semicolon.
    327 |      })
                 ^ Missing semicolon.
    331 |}
          ^ Missing semicolon.

>> 24 errors in 6 files

@jimfleming
Copy link
Contributor

Just posted #57 which includes the jshint fixes and some other random cleanup. You're welcome to copy over to this or we can land both or whatever makes sense. Just wanted to get it up.

@brettlangdon
Copy link
Contributor Author

@jimfleming I merged your branch down into mine, and basically just took my package.json and Gruntfile.js, because I was using grunt-jsfmt rather than grunt-exec, unless you'd rather use grunt-exec so we can run everything through ./bin/jsfmt so the code gets run through the current development version of jsfmt?

EDIT: and I setup .travis.yml to install grunt-cli

@jimfleming
Copy link
Contributor

@brettlangdon Yah, that was my original thought but I'm ok with grunt-jsfmt too.

package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify this to james2doyle/grunt-jsfmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks

@jimfleming
Copy link
Contributor

Oh, right — do you have a jsfmtrc defined somewhere? Is it easy to force defaults when loading the config? We can fix that later too. Its ancillary.

@brettlangdon
Copy link
Contributor Author

@jimfleming I dont :/ it is pulling the default config

@jimfleming
Copy link
Contributor

Hmm, that's odd. $ jsfmt -d examples/styleGuide.js shows nothing for me and $ jsfmt -w examples/styleGuide.js writes no changes.

@brettlangdon
Copy link
Contributor Author

@jimfleming, ah, it is because of grunt-jsfmt, yeah, I'll go back to grunt-exec to do ./bin/jsfmt -w ...

@brettlangdon
Copy link
Contributor Author

@jimfleming moved to grunt-exec and running ./bin/jsfmt ...

@jimfleming
Copy link
Contributor

@brettlangdon it all looks good to me. Can you merge in master one more time then we can merge it in.

EDIT:

We can’t automatically merge this pull request.

@brettlangdon
Copy link
Contributor Author

done.

@jimfleming
Copy link
Contributor

Thanks, will merge when passing.

jimfleming added a commit that referenced this pull request Jun 4, 2014
@jimfleming jimfleming merged commit c419556 into rdio:master Jun 4, 2014
@brettlangdon brettlangdon deleted the grunt branch June 4, 2014 20:14
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.

2 participants