standard / semistandard

:icecream: All the goodness of `standard/standard` with semicolons sprinkled on top.
MIT License
1.41k stars 124 forks source link

Add support for --format #13

Closed bnolan closed 9 years ago

bnolan commented 9 years ago

Fixing up old javascripts by hand sucks.

Flet commented 9 years ago

Cool, will get something going in the next few days. Its been requested here too: Flet/SublimeLinter-contrib-semistandard#2

a quick workaround is to use https://github.com/maxogden/standard-format and add semicolons...

ricardofbarros commented 9 years ago

+1

ricardofbarros commented 9 years ago

I did a fork(it's published as well) of maxodgen standard-format with the semicolons, I was about to add a issue to add support to --format, what a coincidence :smile:

Flet commented 9 years ago

Yes! Please do! PR to add flag will be accepted here too and collab added if you're so inclined!

ricardofbarros commented 9 years ago

But from what I understand to add --format support it should be on standard-engine, or can I extend it on cmd.js ?

Flet commented 9 years ago

We can just write a brand new cmd.js here and add the format flag.

ricardofbarros commented 9 years ago

Sounds good, I can do the PR with the brand new cmd.js

Flet commented 9 years ago

Sweet! Thanks sorry not sure why this got closed :)

gsklee commented 9 years ago

Cool :)

ricardofbarros commented 9 years ago

I did a new cmd.js, but the tests are failing:

⨯ api usage
  not ok 3 error count 2
    ---
      operator: equal
      expected: 2
      actual:   42
      at: zalgoSafe (/Users/rbarros/Desktop/github/semistandard/node_modules/standard-engine/node_modules/dezalgo/dezalgo.js:20:10)
    ...
# clone repos from github
Cloning into '/Users/rbarros/Desktop/github/semistandard/tmp/cursorfun'...
Cloning into '/Users/rbarros/Desktop/github/semistandard/tmp/scenevr'...
✓ clone repos from githubros/Desktop/github/semistandard/tmp/acetate'...
✓ lint repos
# tests 10
# pass  9
⨯ fail  1

Something related to the zalgo protection you have on standard-engine? Now I'm going to ask again shouldn't we tackle this issue in standard-engine ? adding a new arg to options to pass a formatter.

Flet commented 9 years ago

Hmm, have code somewhere?

ricardofbarros commented 9 years ago

No, I will commit to my fork so you can see, in a sec.

ricardofbarros commented 9 years ago

Take a look.

Flet commented 9 years ago

Regarding standard-engine, sure we can add it to the cmd.js there, but it should not necessary to accomplish this for semistandard.

If we want to change standard-engine to take a formatter things start to get weird/complicated. Right now the only input required for is a plain old config object. We would have to also pass it a formatter and possibly set some kind of api that all formatters would be required to follow and possibly field other ways to call a formatter. This additional complexity is not worth it right now in my opinion.

ricardofbarros commented 9 years ago

I do agree with you that it may bring some unnecessary complexity to standard-engine, but only if we get too picky on what kind api the formatters must have so standard-engine understand it.

But the API of the maxodgen/standard-formatter is pretty simple and straightforward, 2 methods .load(opts, cb) and .transform(file), in which the standard-engine only will be using transform, so for compatibility of the formatter API we should only check for 1 method. Seems pretty simple, I dunno that is my opinion. I just don't understand how to solve that error without messing with the tests case :sob:

Flet commented 9 years ago

OK, so part of this was me getting cheeky with the api.js test. What its doing is testing bin/cmd.js, which before only had two lines (and is in standard format). So, its expecting 2 "missing semicolon" errors, but its getting 42 of them now since the file is now a lot larger :)

ricardofbarros commented 9 years ago

So we just need to change the expected output from 2 to 42? Does this affect anything ?

Flet commented 9 years ago

yep! For now just change line 9 of api.js

t.equal(result.errorCount, 42, 'error count 42')
ricardofbarros commented 9 years ago

Tests passed, thank you :smile:, going to submit a PR now.

Flet commented 9 years ago

Published as 4.2.0. Thanks for contributing!

joseluisq commented 9 years ago

@Flet cool :+1: ! So I hope it can be to reflected at https://github.com/Flet/SublimeLinter-contrib-semistandard/issues/2 too.

Flet commented 9 years ago

Ah yeah I think StandardFormat on package control will work, but I'll confirm and update that issue.