Auto formatting API and option. [$315 awarded] #516

Closed
mikesherov opened this Issue Jul 8, 2014 · 83 comments

Projects

None yet
@mikesherov
Contributor

JSCS should have the ability to automatically format a given file with the specified rules. There are several challenges here:

For now, we are only going to consider whitespace changes. This approach is outlined here:

  1. Change all rules to use the JsFile traversal methods instead of directly referencing the token by index directly. This allows us to change the underlying token data structure at will while only having to change the traversal methods.
  2. Make all whitespace only rules use the token-assert framework.
  3. Modify the token list so that it includes comment tokens, and a new terminal token called "EOF".
    3a. Modify traversal methods to ignore comment tokens as necessary.
  4. Modify comment tokens to include a property called "prefix", which contains the preceding whitespace.
  5. Add a function called errors.fixToken (or some other name), which takes in a token, and the updated prefix whitespace, which modifies the prefix of that token.
  6. Make token-assert call errors.fixToken appropriately.
  7. Modify StringChecker so that it can output the modifed source and save it to disk. It's simply a loop over the token list, printing the prefix followed by the value of the token.
  8. Modify CLI to respect a --fix parameter, which triggers the autoformat option.

Please see #514 as one possible implementation that ultimately wasn't how we wanted to approach the problem.

The $315 bounty on this issue has been claimed at Bountysource.

@mikesherov mikesherov added this to the 2.0 milestone Jul 8, 2014
@mikesherov
Contributor

From @gero3:

I've updated the formatter to make it useable.
To test it, I used a minified build of three.js and used the MDCS configuration.
This is the result: I have file that complies with that standard.
https://gist.github.com/gero3/a787fe2023a17ffd6ea8

How does it work:

Get the errors by checking the file.
For each error, check with the source what needs to be changed and create a change request with what need to be removed or what needs to be added as strings.
apply the change requests
until there are no errors (or retry 10 times) repeat this cycle.
It is a fairly easy scheme which is fast enough for small changes and normal files.

@sindresorhus
Member

👍 This would be incredible! There are formatters like esformatter/fixmyjs, but JSCS has the absolute best rules and momentum. With #341 and this, JSCS is going to be the tool I've always longed for since I started with JS.

@mderazon
mderazon commented Oct 6, 2014

👍 agree with @sindresorhus

@addyosmani
Contributor

In case it would help, I (and I'm sure others) would be happy to back this through BountySource. If that's less useful, 👍. There's huge value in having something more concrete than fixmyjs available.

@mdevils mdevils changed the title from Auto formatting API and option. to Auto formatting API and option. Oct 7, 2014
@mrjoelkemp
Member

Just an FYI, @addyosmani and others, we recently added Bountysource support.

@mdevils mdevils changed the title from Auto formatting API and option. to Auto formatting API and option. [$50] Oct 8, 2014
@mdevils mdevils added the bounty label Oct 8, 2014
@gero3
Contributor
gero3 commented Oct 8, 2014

Wouldn't it be easier if you only need edit source, token list or AST and regenerate the other 2?? It would be a lot easier to get all edge cases correct. Also this would make the rules simple for formatting. Another advantage is that we only need to keep in mind what whitespace does in one of them.

As for the API, I would assume a new command:

  • jscsformat path[ path[...]] --config=./.config.json
  • jscsformat path[ path[...]] --preset=jquery
  • jscsformat path[ path[...]] --verbose
  • jscsformat --help
  • jscsformat --version

The rules itself would get an extra function 'format' that has as parameters a file and an error.

The formatting should be done after checking the source. That way, the errors are a great way to communciate between the formatting and checking. It also makes sure that there is a seperation of those 2 jobs.

The reason I'm against only considering rules that don't change the ast because that would make it hard to add it later. it would seem like a hack.

That is why the following scheme is (I think) a good choice because of its simplicity and its ease of use.

  • The first thing is to get the errors of the files.
  • For each of these rule we call the format method of the corresponding rule:
    • This format function creates change requests based on the original source code and the error.
    • These change requests consists of 3 properties:
      • A startIndex: to indicate where the change requests should start based on the original source code.
      • A removeCharCount: to Indicate how many characters should be removed after the startIndex of the changerequest:
      • A charsToAdd: to indicate which character should be added after the removal.
    • These change requests should always be centered around the original source code.
  • After getting every change request, it is time to generate the source code.
    • To do this, we sort the change requests from the lowest start index to the highest.
    • for each of these change requests, the result each time increases with:
      • The original source code that is in between the previous startindex + removedchars and the start index of the change request.
      • the charsToAdd from the change request.
  • After all of this we check again for errors:
    • If there are errors repeat the same cycle (we should really limit this to make sure we don't get in an endless loop)
    • If there aren't any errors any more, we've succeeded.
    • If we run against an esprima error, then we should reverse to the original source code and end.

The main reasons I prefer this method is:

  • The source code is easier to work with:
    • The token list has 4 different actions, adding, removing, lengthen and shorten a token while source code only has 2 actions removing and adding.
    • The Ast has even more actions and would have a lot of edge cases.
  • I first started with editing directly on the source code starting from the errors with the highest line,column, but this wasn't the best indicator to see what needed to change in what position. Mainly when an error meant adding 2 things like the require-parentheses-around-iife rule.
  • The rules don't even need to know what the other rules changed.

PS: sorry for the long comment. But I wanted to jump start the discussion with what i had already made in the previous pull request. For an example of the code, https://github.com/gero3/node-jscs/tree/formatter.

@mdevils mdevils changed the title from Auto formatting API and option. [$50] to Auto formatting API and option. [$100] Oct 8, 2014
@mdevils mdevils added the bounty label Oct 8, 2014
@mikesherov
Contributor

@gero3, thanks for the explanation. Glad to see you contributing again after a short break.

I'll have a more thorough response later, but essentially, I want to make sure that corrections are made progressively.

Most of your answer, at first glance, sounds like a brute force approach. It may be the one we end up going with, but I want to entertain more ideas as well.

@mdevils, did you have thoughts on how we would accomplish this? If so, please post your thoughts. I'll write up mine tomorrow.

@mrjoelkemp
Member

Thanks @gero3 for the details. Great ideas.

Some initial thoughts:

I envisioned this functionality as a plugin (separate repo, possibly using ideas from #335) that takes in an errorList and generates deltas for every error (aggregated and applied to the source's ast). The tool would have custom delta-generation logic for every jscs rule (perhaps even a plugin system of its own for jscs' 3rd party rules/plugins).

I agree that formatting and error-checking are two separate processes, but they should not live together (as your initial implementation described). I fear that we'll scare off contributors of new rules if they need to worry about providing formatting logic as well.

As an aside, It would be great to get esformatter on our side and in the jscs-dev repo: to use our rules and align with our trajectory. Perhaps @mikesherov can reach out and do some smooth talking.

@mikesherov
Contributor

I think that autoformatting MUST be part of the core. To move this
functionality to a plugin for fear that it discourages contributions seems
backwards to me. Our primary concern is our users first and our
contributors second.

This is something the users want in core and have asked for quite
forcefully.

Esformatter is a great tool, but its configuration model and strategy is
quite different from us. I'd love if we could combine efforts, but using it
inside of JSCS means double parsing everything and duplicated logic for
everything.

Mike Sherov

@gero3
Contributor
gero3 commented Oct 9, 2014

@mrjoelkemp

I've also been thinking about separating this functionality. But I had some reservation about it.

  • Mostly the formatting is dependent on the checks. If a check changed, the formatting has to be updated.
  • Seperated, The AST needs to be generated twice if we want to keep it completely seperate.
  • Enforcing that formatting is needed, was also not my intention. If formatting for a rule isn't provided, then we shouldn't try to format it. The reason for not enforcing formatting in a rule is simply because not every rule can be autoformatted. For instance, a rule that checks that eval can't be autoformatted.
@mrjoelkemp
Member

I think that autoformatting MUST be part of the core. To move this
functionality to a plugin for fear that it discourages contributions seems
backwards to me. Our primary concern is our users first and our
contributors second.

@mikesherov It is possible to incorporate the tool in core without merging codebases.

Our primary concern for JSCS is style checking and auto-formatting is secondary – though very important and this tool needs to exist (preferably within our organization). It does not need to exist as part of core. In a similar argument, should JSHint absorb fixmyjs' codebase into their core? No. Both tools can live separately and harmoniously.

Deterring contributors is another way of saying taking on too many concerns within a single tool is adding unnecessary amounts of complexity to the codebase. My fear is that this will raise the barrier to entry.

Coupling both tools means that no breaking changes to our rules can be made without also updating the formatting code. As two separate (but synergistic) tools, jscsformat can gradually upgrade to newer versions of jscs.

The AST needs to be generated twice if we want to keep it completely seperate.

@gero3 Not necessarily. As a plugin (or filter), we should discuss core supplying lib/js-file objects in addition to errors. Perhaps @markelog and @mdevils will have some ideas on this communication layer.

@mdevils
Member
mdevils commented Oct 9, 2014

I have proof-of-concept generator code from tokens. It works. I doubt we need AST tree to generate the code: it is far more complex.

@mdevils
Member
mdevils commented Oct 9, 2014

Right after I finish with plugins I'm going to show an autofixing example. It is easy and requires minimal rule rewrite.

@mikesherov
Contributor

Yes, we should be using the tokens. However, there is work being done on
esprima itself to make this even easier.

I'm excited for a proof of concept that I can go to Ariya with in case
there's any pain points.

On Thursday, October 9, 2014, Marat Dulin notifications@github.com wrote:

Right after I finish with plugins I'm going to show an autofixing example.
It is easy and requires minimal rule rewrite.

Reply to this email directly or view it on GitHub
#516 (comment).

Mike Sherov

@markelog
Member

So excited about this :-)

@hzoo hzoo referenced this issue Oct 19, 2014
Closed

New format attempt #698

@markelog
Member
markelog commented Nov 5, 2014

I believe it should be a separated npm-module, (see similar reasons in #747)

Also we would need more information from the errors, like range resource is absolutely essential, we could do that either with new method of errors module or with new errors module altogether - #453, if new module is used it would collect more data and then send it to errors instance in order to preserve back-compat, if need to be that is.

@mikesherov
Contributor

I don't think it should be a separate module. There are ways to achieve this that you're not thinking of. Give us a chance to show you. Both @mdevils and I have solutions we'd like to try for a large swath of the rules.

@markelog
Member
markelog commented Nov 5, 2014

I don't think it should be a separate module.

Base parts of it could be, so it would be used somewhere else besides jscs, we're only about code style, but someone could use it for other things. That is the only reason why i think this should be separated.

There are ways to achieve this that you're not thinking of.

You mean without range resource?

@mikesherov
Contributor

Base parts of it could be

Sure, I misunderstood. I thought you meant not to be part of JSCS core. But you're saying that it's a module that JSCS core uses.

Perhaps, but I think we should focus on modularization a bit later. Features first.

you mean without range resource?

It depends. The way I had though about doing this in my mind actually involves augmenting the token list so that it contains comment tokens, has an additional token at the end to represent EOF, and that every token gets a "whitespaceBefore" property that is the literal whitespace that comes before the token.

This would become a part of the new standard for CST "concrete syntax tree", and eventually this work would be done by esprima. Furthermore, the actual tree would link to the token list with startToken and EndToken so that seamless switching between the tree and tokens would happen.

We'd start by doing this in JSCS and promoting it to esprima and other parsers, hopefully making it a community standard.

Now, given that we have a token list that contains comments and whitespace, most autoformatting operations and just changing the "whitespaceBefore" property of a token.

At the end of all modifications, you generate source by doing:

tokens.reduce(function(accum,next) {
  accum += next.whitespaceBefore + next.value;
}, '');
@mikesherov
Contributor

/cc @mdevils. I'd like to know what your approach here is going to be because I'd also like to take a crack at mine.

@markelog
Member
markelog commented Nov 5, 2014

Perhaps, but I think we should focus on modularization a bit later. Features first.

Sure, moving it to the its own module is an easy part.

It depends. The way I had thought...

I was taking about a different thing, right now, if rule find an error it would just pass its line and column to errors instance. But in order to fix things we need at least range data to identify the place in tokens array for further actions on it.

Tokens array should be augmented, no question on that.

@mikesherov
Contributor

But in order to fix things we need at least range data to identify the place in tokens array for further actions on it.

Or at least a list of tokens to modify (if they contained whitespace info). But yes, something more than line and column are needed.

@mdevils
Member
mdevils commented Nov 5, 2014

I'm against separated module. It would incredibly increase complexity. I have implementation ideas and I have already show you some. This week I am going to work on better assertions (assert.spaceBefore(...)), which will allow as to implement auto-fixing for free.

@zxqfox
Member
zxqfox commented Nov 5, 2014

I've thought about this issue and have got some ideas but already see that @gero3 already said about it.

So I think the simplest way is return somehow from rules what to change (insert/update/remove), it can be done e.g. via levenshtein algorithm. It looks much easier when you compare the difference between non-formatted and formatted code:

// unformatted sample
function yolo ( var1, var2 ){
return 1;
};

// formatted sample
function yolo(var1, var2) {
    return 1;
}

While the difference list between them will be:

[
    {"type": "remove", "loc": {"start": {"line": 1, "column": 13}, "end": {"line": 1, "column": 14}}},
    {"type": "remove", "loc": {"start": {"line": 1, "column": 15}, "end": {"line": 1, "column": 16}}}, // here's the problem cuz column will be changed after first remove
    {"type": "remove", "loc": {"start": {"line": 1, "column": 26}, "end": {"line": 1, "column": 27}}},
    {"type": "add", "loc": {"start": {"line": 1, "column": 28}}, "value": " "},
    {"type": "add", "loc": {"start": {"line": 2, "column": 0}}, "value": "    "}
}

E.g. for replaces it will be something like:

[
  {"type": "replace", "loc": {"start": {"line": 1, "column": 15}, "end": {"line": 1, "column": 16}}, "value": "xxx"}
]
// or separated
[
  {"type": "remove", "loc": {"start": {"line": 1, "column": 15}, "end": {"line": 1, "column": 16}}},
  {"type": "add", "loc": {"start": {"line": 1, "column": 15}}, "value": "xxx"}
]

Right?

@markelog
Member
markelog commented Nov 5, 2014

I think there is a lot of ideas flying around, we need to play with some code

@mdevils mdevils referenced this issue Nov 5, 2014
Merged

Autofixing: assertions #751

4 of 6 tasks complete
@mdevils
Member
mdevils commented Nov 5, 2014

Started to work on the issue: #751

@mdevils mdevils changed the title from Auto formatting API and option. [$100] to Auto formatting API and option. [$200] Nov 11, 2014
@mdevils mdevils added the bounty label Nov 11, 2014
@indexzero
Contributor

Doubled the bounty on this to $200. Will be so useful in so many ways.

@mikesherov mikesherov modified the milestone: 1.9, 2.0 Nov 29, 2014
@mikesherov mikesherov self-assigned this Nov 29, 2014
@markelog markelog modified the milestone: 1.9, 1.later Jan 8, 2015
@jzaefferer
Contributor

So #751 was merged, introducing the infrastructure for autofixing, but not the autofixing itself, is that right? Does someone have plans to work on the next step? If not, would a bigger bounty make a difference?

@mikesherov
Contributor

Yes, it's the next thing im working on. Bounty makes no difference here.

@indexzero
Contributor

That's awesome @mikesherov! I didn't realize #752 was merged. Is there a running list of rules that need formatters? Now that the API is defined I could help write a few.

@soliton4

hi there
i found this in my inbox:
soliton4/nodeMirror#18

would be a totally cool feature, but it looks like the formatting option is still under construction.
is there any part of it usable and can you point me to a working piece of code?

i basically need the option to input text and get formated text as output.

thank you very much ;D

@jdlrobson
Contributor

Hey @mikesherov I'm super interested in this too. Is there any branch I should be watching where this is happening? I would be interested to see how you are thinking about this and see if I can help out!

@mikesherov
Contributor

@jdlrobson and @indexzero, the most helpful thing you can do is review all existing rules and make sure spacing and line break rules are all using the file.assert methods. Those will be the first to get fixed.

@mikesherov
Contributor

Just updated the description with full outline of the approach:

  1. Change all rules to use the JsFile traversal methods instead of directly referencing the token by index directly. This allows us to change the underlying token data structure at will while only having to change the traversal methods.
  2. Make all whitespace only rules use the token-assert framework.
  3. Modify the token list so that it includes comment tokens, and a new terminal token called "EOF".
    3a. Modify traversal methods to ignore comment tokens as necessary.
  4. Modify comment tokens to include a property called "prefix", which contains the preceding whitespace.
  5. Add a function called errors.fixToken (or some other name), which takes in a token, and the updated prefix whitespace, which modifies the prefix of that token.
  6. Make token-assert call errors.fixToken appropriately.
  7. Modify StringChecker so that it can output the modifed source and save it to disk. It's simply a loop over the token list, printing the prefix followed by the value of the token.
  8. Modify CLI to respect a --fix parameter, which triggers the autoformat option.

@jdlrobson @indexzero help with 1. and 2. would be greatly appreciated.

@mdevils mdevils changed the title from Auto formatting API and option. [$200] to Auto formatting API and option. [$300] Feb 6, 2015
@mdevils mdevils added the bounty label Feb 6, 2015
@TheSavior TheSavior added a commit to TheSavior/node-jscs that referenced this issue Feb 8, 2015
@TheSavior TheSavior Updating rules to use File Traversal APIs.
Finishes step 1 in auto format steps outlined in #516
22c8b38
@TheSavior
Contributor

@mikesherov, is referencing lines by index a problem? There are also a few rules that are using _operatorIndex (I'm not sure what this is).

@mikesherov mikesherov added a commit that referenced this issue Feb 8, 2015
@TheSavior @mikesherov TheSavior + mikesherov Updating rules to use File Traversal APIs.
Finishes step 1 in auto format steps outlined in #516

Refs #516
Closes gh-1016
394edc9
@mikesherov
Contributor

@TheSavior, operatorIndex is unrelated. Good question though.

@soliton4
soliton4 commented Feb 8, 2015

i dont know if this is any help to you but i post it anyway.
nodeMirror has a early version of the formatter integrated now. jscs can also be selected as linter. that way its easy to see how well the formatter works.

let me know if this was helpful

also keep me updated if i should pull a new version of the formatter

@mdevils
Member
mdevils commented Feb 15, 2015

Here wo go: #1058

@mikesherov
Contributor

@mdevils, let's track what's left:

  1. Saving file in case of not stdin.
  2. Autofixing for validateIndentation.
  3. Strategy for non whitespace fixes.
  4. Converting remaining rules.

Anything else?

@TheSavior
Contributor

Can you give an example of a reasonable non whitespace thing we could fix?

@mikesherov
Contributor

@TheSavior the multipleVarDecl rules. We don't need to solve it now, but we have the come up with something like @gero3's strategy of direct source manipulation followed by AST rebuild or figure out how to manipulate an existing AST.

@TheSavior
Contributor

My intuition says that all of the iterative solutions we are considering are the easy first approach, but won't be robust long term for all possible 'fixes' rules might want. I would think we need a way to have some sort of engine that understands what the rules are doing and could know if any of the rules conflict.

At that point, we would be able to take in a parse tree of a file and step through it applying the applicable rules and convert it step by step into actual code.

@mdevils
Member
mdevils commented Feb 15, 2015
  1. Saving file in case of not stdin.

What do you mean? Current implementation saves changes into files.

@mikesherov
Contributor

I must've missed that!

@zxqfox
Member
zxqfox commented Feb 15, 2015

Strategy for non whitespace fixes.

It can be fixing in place if we already doing it in assertion. But I feel like it becomes a mess because of high cyclomatic complexity in some rules.

@mdevils
Member
mdevils commented Feb 15, 2015
  1. Strategy for non whitespace fixes.

I think we can leave this as is and focus on whitespaces first.

@TheSavior
Contributor

👍

@hzoo hzoo added a commit to hzoo/node-jscs that referenced this issue Feb 17, 2015
@hzoo hzoo Autofixing: add more rules to use assertion framework
Refs #516
Closes gh-1075
5eec32c
@hzoo hzoo added a commit to hzoo/node-jscs that referenced this issue Feb 17, 2015
@hzoo hzoo Autofixing: add more rules to use assertion framework
Refs #516
Closes gh-1075
712d53f
@hzoo
Member
hzoo commented Feb 17, 2015

Do we have a strategy for disallowMixedSpacesAndTabs or disallowTrailingWhitespace? Also whitespace related to alignment like requireAlignedObjectValues.

Other whitespace rules (should be the last ones)

  • disallowMultipleLineBreaks
  • disallowSpaceAfterLineComment, requireSpaceAfterLineComment
  • requireLineFeedAtFileEnd
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 19, 2015
@mikesherov mikesherov JsFile: add getLineBreaks function to support future whitespace fixes
Refs #516
b78b447
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 19, 2015
@mikesherov mikesherov validateIndentation: autofixing!
Fixes #1085
Refs #516
6e4e494
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 21, 2015
@mikesherov mikesherov validateIndentation: autofixing!
Fixes #1085
Refs #516
bf2ea5b
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 21, 2015
@mikesherov mikesherov validateIndentation: autofixing!
Fixes #1085
Refs #516
29b4dd7
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 21, 2015
@mikesherov mikesherov validateIndentation: autofixing!
Fixes #1085
Refs #516
2f3e33e
@mikesherov mikesherov added a commit that referenced this issue Feb 22, 2015
@mikesherov mikesherov Assertions: add tests for linesBetween
Refs #516
Refs gh-1097
b54d193
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 24, 2015
@mikesherov mikesherov disallowSemicolons: do not yet allow autofixing, which is a non-white…
…space change

Refs #516
Refs #1089
edcf36e
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 24, 2015
@mikesherov mikesherov disallowMultipleLineString: do not yet allow autofixing, which is a n…
…on-whitespace change

Refs #516
Refs #1089
b615013
@millermedeiros

so @jzaefferer just told me about the plan to add formatting to jscs. To be really honest, I would love to see someone else solve this problem for me since I'm not having that much time to work on esformatter and have other priorities in life at the moment... but for what I could see you guys are moving towards a dead end and underestimating the complexity of this problem.

Handling white space is the easiest part of code formatting by far; adding/removing line breaks is tricky (child nodes affects the parent nodes structure) but indentation is the real challenge; specially if you want to support multiple code formats.

There are many reasons why esformatter works the way it does, and today I added some info describing how/why we transform and indent the code that way; it might be worth the read.

I did not spend too much time looking at how you're implementing the autofix but it looks like most of the rules are line based, if that is really the case you will have a really hard time when you try to add/remove line breaks and implement proper indentation. There are so many edge cases that it's basically impossible to handle it all in a single step (indentation should always be the last step otherwise it will be wrong for sure).

I would love if more people contributed to esformatter (fixing what is currently broken) and actually created a plugin to convert the jscs config file into a format that esformatter can understand. I honestly think it would save a lot of headaches to all the people involved.

Nowadays I would even implement esformatter in a different way but some changes would require so much work that it is definitely not worth doing (like building a proper CST instead of using an enhanced AST).

Until I find something that works better than esformatter, or stop using JavaScript, I'll keep developing it, even if I take forever to finish... - any help is highly appreciated, pull requests with reduced test cases and bug reports are extremely helpful (code is only as good as the tests).

PS: using the assert is really the best abstraction that I could think about, it's very similar to how I handle it on esformatter with ws.limitBefore(token, tokenIdOrValue) and ws.limitAfter(token, tokenIdOrValue) - limiting line breaks is way more complex since you might have comments and white spaces in between but I also abstracted the logic the same way...

PPS: I also think the responsibility of fixing the code should be handled by a separate lib (single responsibility FTW!) but there are definitely drawbacks for both approaches.

Good luck!

@TheSavior
Contributor

@millermedeiros Not to speak for the rest of the team, but I really appreciate you coming to provide us this context. Having built esformatter, you definitely have some specific understanding that we may not have.

The assertions we use and having that add whitespaceBefore, as you say, seems pretty consistent with how you have things outlined in the file you linked.

Something I didn't quite get from your document explaining how esformatter tokenizes and how you guys transform, are specific examples.

I'd love to get some more understanding and specific examples that you've run into issues with once we try to tackle indentation. I don't believe we have taken any steps in that direction yet.

@millermedeiros

@TheSavior there are a lot of edge cases related to indentation, specially for complex nodes like IfStatement, FunctionExpression and CallExpression

There is also the problem of chained member expressions and how they should be handled, specially if you think about the jQuery style guide when special methods cause negative indents...

There are some crazy rules about what should be start/end of the indentation in some cases, specially since we might have optional parenthesis and curly braces...

I'm nailing down one bug at a time, but some problems are very hard to solve if all you have is the line number and/or a vague AST structure. In many cases you need to understand the actual code context and work around the AST limitations.

@millermedeiros

@TheSavior our token list is a LinkedList and not an Array, so it's very trivial to add/remove items (we don't use the context of indexes.. we just edit the LinkedList in place) - every token have a reference to the prev and next tokens, so to add/remove you just need to update the references to prev/next on the adjacent tokens. Every node on the AST also have a reference to the startToken and endToken to make it easier to traverse.

@mikesherov
Contributor

@millermedeiros, you're welcome to help help us solve the problem here in JSCS just as much as we're able to help you in esformatter. I appreciate the concern about the dead end, but I think it's most unfounded if you haven't taken the time to read all we've said about it on our mailing list and in our various issues. If you're truly interested in having someone else solve the problem, and think that esformatters current impl is suboptimal, we should work together here in JSCS to get this done. In regard to specific critiques:

Linked list for token insertion is a must. We just aren't there yet. Our abstraction makes it pretty trivial to add when we finally tackle that problem. Note that we're also working with Esprima and the estree project to attempt to move forward the CST at some point soon so that something like rocambole or what we do now (basically the same work) is available directly from an esprima parse.

As far as indentation goes, I think I have a pretty good grasp on the edge cases, and I'd invite you to read what my existing pull request for autofixing indentation tries to accomplish. It has matured over a long period of time and is fairly lenient in what it enforces. Would love specific feedback there.

@mikesherov
Contributor

@millermedeiros, I'd also like to mention that the negative indentation problem for chained method calls is one of concern but is literally no different then any something like switch/case indentation.

Again, I'd invite you to check out our validateIndentation rule and see how it works before you dismiss it.

With regards to having to do indentation last, that's also a bridge that we may have to come to, but I doubt it. As long as the rules that modify number of lines preserve indentation, it shouldn't matter. But again, we'll see when we get there, and I think JSCS's infra is robust enough to handle that chabge should it be needed to do it last (which is something I suspected, but am not convinced).

@mikesherov
Contributor

@millermedeiros, lastly, I want to just say that personally, we could use your help on JSCS. Is love for us to consider merging the two projects.

No one is telling you to give up on esformatter, and you should continue working on it to your heart's content, but I've seen you express doubt to as to its design and lament its pace of development.

We've tried to build an inclusive, contributor friendly codebase here, and I think JSCS's momentum and contributor base make it the project we should all be working on instead of in esformatter.

Again, you're welcome to do whatever you like, and we appreciate the input and feedback you've given us, so even if you decide to continue developing esformatter into the foreseeable future, no big deal. But I think we could solve more problems together rather than apart and I think JSCS is in a better place at the moment to handle the task. Of course, I could just be biased.

Let me know what you think.

@millermedeiros

@mikesherov I feel glad for the invite to help on jscs and join forces but I will probably just keep working on esformatter instead. Time is a very limited resource and there are different user needs to be filled.

The design flaw I regret most on esformatter is that I should have built a real CST since the beginning with good helpers for code manipulation; making sure that the CST was as easy as possible to modify (giving more power to plugin authors) and with saner node boundaries - having to search for all the tokens is cumbersome and parenthesis makes things weirder. The main reason why I did this way was because I thought that the tool would only handle white spaces and line breaks, but current plugins are showing that people actually want it to fix other issues as well (quotes, braces, semicolon, rearrange the code, etc). - jscs structure is no different from that and while you implement code formatting it should become even more similar (eg. start using linked lists) - I should have also delegated more work to plugins and/or separate libs since the beginning.

We are basically approaching the problem starting from 2 opposite directions - I was initially thinking about writing a tool that does basic string diff between esformatter input/output to validate the code style, while jscs uses the validation to correct the differences...

I'm confident you guys will find a solution to all the problems given enough time, just wanted to give a heads up; keep the good work!

PS: Maybe this idea of having a competitor will motivate me to move faster :D

@mikesherov
Contributor

@millermedeiros thanks for the thoughtful reply. At the very least, perhaps you can help contribute to the CST discussion in the estree repo once that gets underway... as the atuhor of Rocambole, your input is invaluable there.

@millermedeiros

FYI I resumed esformatter development and closed 20 issues this week (all the reported bugs that we had) and just published v0.5.0 to npm. v0.6 is coming soon with lots of improvements. I promise I won't spam in the next releases, just thought it was a good idea to share in case you guys change your minds and decide to reuse it (or at least decide to borrow some of the test cases). cheers.

@mikesherov
Contributor

Congrats. Good luck with the continued development of your project.

@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 27, 2015
@mikesherov mikesherov JsFile: add getLineBreaks function to support future whitespace fixes
Refs #516
d6ec3bc
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 27, 2015
@mikesherov mikesherov validateIndentation: autofixing!
Fixes #1085
Refs #516
4683329
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 27, 2015
@mikesherov mikesherov TokenAssert: ensure linesBetween preserves indentation
Refs #516
Refs #1085
96b9e9c
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 28, 2015
@mikesherov mikesherov validateIndentation: intelligently indent object expressions
Refs #516
Refs #1085
a8689ff
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 28, 2015
@mikesherov mikesherov validateIndentation: ensure empty comment lines are updated correctly
Refs #516
Refs #1085
4cf4df5
@mikesherov mikesherov modified the milestone: 1.12, 1.later Feb 28, 2015
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 28, 2015
@mikesherov mikesherov tokenAssert: rename getlineBreak to getLineBreakStyle
refs #516
235867c
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Feb 28, 2015
@mikesherov mikesherov validateIndentation: autofixing!
Also includes update to tokenAssert to preserve indentation when
augmenting line count as this would otherwise throw off indentation.

Refs #516
Refs #1121
Closes #1073
Closes #1085
Closes gh-1087
d1d6ff4
@TheSavior
Contributor

I just put up a sublime plugin that uses the globally installed JSCS version to autoformat your code.

https://github.com/TheSavior/Sublime-JSCS-Formatter

I'm going through the process to get it added to Sublime's Package Manager right now: wbond/package_control_channel#4148

@varemenos
Contributor

@TheSavior you truly are the savior :)

@mikesherov
Contributor

@TheSavior, awesome work! Would you mind waiting until 1.12 is released before submitting that package? I wouldn't want users to get confused.

@TheSavior
Contributor

@mikesherov Sure thing. This is my first sublime plugin and I really have no idea what I'm doing, so I wanted to jump through the hoops now to get things cleaned up.

If any of you use sublime and know python or want to explore the world of plugins, I would really appreciate any pull requests people have time for. :)

@TheSavior TheSavior referenced this issue in SublimeLinter/SublimeLinter3 Mar 1, 2015
Open

Enable a Linter to Support Autofixing (JSCS) #207

@indexzero
Contributor

@mikesherov looks like great progress here! I read the comments a bit and saw all the referenced commits for each rule 👍

... is there a place listing all the rules and which ones still need autofixing added? A checklist somewhere would be super helpful. I've finally got a little time and would love to pinch hit and knock a few out.

@markelog
Member

@indexzero only whitespace, eof and indentation rules are supported at the moment, there is no clear way to support others, more specific rules like disallowAnonymousFunctions or disallowCommaBeforeLineBreak, etc.

Right know, we thinking, either specific assertions for every rule or this.

@mikesherov
Contributor

@indexzero disallowMultipleLineBreaks still needs conversion.

Also, the (no)whiteSpaceBetween rule need to be fixed so that if a comment token exists between the two provided tokens, don't attempt to autofix. See the linesBetween rules for prior art there.

If you can get PR's going for that, I think we're in a fairly good place, and can discuss releasing 1.13, which has the goal of fixing whitespace only.

@mikesherov
Contributor

@indexzero also discovered requireAlignedObjectValues needs conversion as well.

@TheSavior
Contributor

@mikesherov Are changing those 4 rules all that is left before releasing 1.13?

@mdevils mdevils changed the title from Auto formatting API and option. [$300] to Auto formatting API and option. [$315] Mar 20, 2015
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Mar 23, 2015
@mikesherov mikesherov Token Assert: normalize whiteSpace assertions to match line assertions
Refs #516
b3f2516
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Mar 23, 2015
@mikesherov mikesherov DisallowMultipleLineBreaks: use assertions
Refs #516
41042e9
@mikesherov mikesherov added a commit to mikesherov/node-jscs that referenced this issue Mar 23, 2015
@mikesherov mikesherov RequireAlignedObjectValues: use assertions
Refs #516
51bd7a5
@gvn gvn referenced this issue in MozillaFoundation/mofo-style Mar 23, 2015
Closed

Deprecate JSBeautify #19

@jacquerie jacquerie referenced this issue in inveniosoftware/invenio Mar 31, 2015
Open

RFC: JavaScript Code Style #1769

@markelog markelog added the autofix label Apr 2, 2015
@markelog
Member
markelog commented Apr 2, 2015

Autofix has been landed, other updates will be iterative, so if no one object, i would like to close this one. Will wait until tomorrow.

As a team, we decided to donate this money to charity organization "Girls Who Code" - https://girlswhocode.com.

Thank you everyone!

@zxqfox
Member
zxqfox commented Apr 2, 2015

Great news everyone 👍

@soliton4
soliton4 commented Apr 2, 2015

i agree

is there a api documentation available yet?

@zxqfox
Member
zxqfox commented Apr 2, 2015

@soliton4 Docs in progress: #1231 #1192

There are the very first try also: #1202

@difosfor
difosfor commented Apr 8, 2015

Experimenting with this in our build tool and very excited about the possibilities! But now of course we want more ;)

What about non-whitespace fixing? I imagine/hope it shouldn't be too hard to fix errors related to comma's and semi-colons. E.g: anything where the error message is already pointing exactly at where a certain character is missing or should not be.

Are you guys working on this already and should we just add some bounty and/or how can I contribute?

PS: Personally I disagree with people who would like to see this developed in a separate project. Fixing is a core feature. Having software tell you exactly what you did wrong and then subsequently not helping you save time by fixing it seems a bit masochistic to me. It's just like an annoying teacher who keeps hitting you on your fingers every time you make a mistake instead of an assistant that smoothes things out for you.

@zxqfox
Member
zxqfox commented Apr 8, 2015

@difosfor #1202 — I think the battle begin ;-)

@markelog
Member
markelog commented Apr 8, 2015

@difosfor #1113 has potential, but there is concern of providing API for modifying AST, since there is obvious danger in that way.

We have this - #1086 as at least one element of protection from ruining your code.

We need to be very careful here.

@mrjoelkemp
Member

@markelog can we close this?

@mrjoelkemp
Member

This solution was worked on by many individuals in the community. Together, we have decided that the bounty should go to charity: https://groups.google.com/forum/#!topic/jscs-dev/u-Gy0Yh97EU.

As a core team member, I was nominated to handle claiming the bounty, donating the money, and posting a donation receipt on the issue to notify the community of the donation. I will post status updates on this issue to stay transparent until the donation receipt is posted.

To start the claiming of the bounty, I need to close the ticket.

@mrjoelkemp mrjoelkemp closed this May 14, 2015
@mrjoelkemp
Member

image

The bounty will be paid out if all backers accept your claim or if no backers reject your claim by May 28, 2015 8:19 AM (GMT).

I'll check bountysource on the 28th (or sooner if all backers accept the claim) to proceed with the next steps.

@mdevils mdevils changed the title from Auto formatting API and option. [$315] to Auto formatting API and option. [$315 awarded] May 28, 2015
@mrjoelkemp
Member

The bounty was awarded.

image

I've requested to be paid via paypal.

image

Bountysource took a $31 cut from the funds. That left a balance of 284 (315 - 31).

@indexzero
Contributor

👍💪🙌

@mrjoelkemp
Member

image

Getting it transferred to my bank and will then donate that money. Everyone wants a cut of the funds, it seems. Not even charity gets a break from fees!

Will post a donation receipt once that's done.

@mrjoelkemp
Member

Just a quick update. The money has reached my account. I'll donate it on Wednesday and post a receipt.

@mrjoelkemp
Member

Donation complete. Thanks again to everyone who contributed to the effort.

donation

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