Add "fix" field in rule declaration

[markelog]

Addresses #1201 #1202 #1265, but most importantly #1113.

Example with validateQuoteMarks is just that, example.

This pull represents two different ideas for autofix infrastructure - "fix" field and assertion interface.

Which is bad, we need to make them complimentary to each other or unite approaches, but we need to keep ball rolling need to release 2.0 soon, we can improve/refactor later.

[coveralls]

Coverage increased (+0.03%) to 99.32% when pulling b158f85 on markelog:fix into 4a0322b on jscs-dev:2.0.

[qfox]

I think better to make a custom object instead of generic one with concrete order of fields. Hidden classes, etc.

[markelog]

Perhaps you mean errorInfo argument of the cast method? Specifically additional property?

In such case, yes, that would be preferable, but it has to be polymorphic since we don't know in what way rule would want apply the fix :-(

[qfox]

Yeah, this place is not the best to talk about hidden classes and performance. errors.cast can do the job.

Personally I'm trying to keep it in mind. Ok, nvm!

[qfox]

Looks good at all ;-)

[mdevils]

I believe we should not introduce second way of fixing. And allowing to modify AST directly is too dangerous for the compatibility.

[markelog]

And allowing to modify AST directly is too dangerous for the compatibility.

See the

This pull represents two different ideas for autofix infrastructure - "fix" field and assertion interface.

Which is bad, we need to make them complimentary to each other or unite approaches, but we need to keep ball rolling need to release 2.0 soon, we can improve/refactor later.

And allowing to modify AST directly is too dangerous for the compatibility.

That will happen, one way or another, we has examples of it in our repo and there is tons of modules already doing that, if we don't provide the way ppl will use other means.

As of use of token-assert module, i believe we already had discussion about that - #1265 (comment). So without going in circles, we have two options, either "fix" function or doing the autofixing in check function.

[mdevils]

Thank you for your explanation. I will try to explain the main problem I see with this approach:

Right now almost all the fixes are made in assertion module. And those fixes are hidden, they are just part of assertion module implementation. Today we are operating on token list and assertion module modifies token list. But token list does not allow us to do a lot of other required changes. For instance, you cannot replace var x = 1, y = 2; with var x = 1; var y = 2;. To apply these changes you need tree. At some point I believe we will start operating on the tree and assertion module will just smoothly switch from token list to tree (because we have good connection between tokens and tree nodes). And during that switch we will not have to touch rules. Both our rules and custom rules will keep working. But if we go and introduce modifying token list directly from the rules, we will tie ourselves into token list and we will not be able to use tree for source modification.

I would suggest:

• Remove fixing logic from check method in validateQuoteMarks and move it to assertion module.
• Add more methods to assertion module. We may even split it to several modules.
• Start actively thinking about new tree, which would allow us to apply modifications. Or start our own tree if https://github.com/estree/estree project is not going to finish in the next several months (For me it looks like they are stuck).

Thank you.

/cc @mikesherov

[markelog]

But token list does not allow us to do a lot of other required changes. For instance, you cannot replace var x = 1, y = 2; with var x = 1; var y = 2;. To apply these changes you need tree. At some point I believe we will start operating on the tree and assertion module will just smoothly switch from token list to tree (because we have good connection between tokens and tree nodes). And during that switch we will not have to touch rules. But if we go and introduce modifying token list directly from the rules, we will tie ourselves into token list and we will not be able to use tree for source modification.

Right now this looks as speculation, need proof of concept. As of tree modification we can provide means for that.

If you have specific rule fix method which would lay in the assert-module you still need to change it to work with tree, so there is no difference where fix method would be specified - in the other module or in the fix field.

Basically, this is why i insist on trying it out, to see how will people will work with it.

Remove fixing logic from check method in validateQuoteMarks and move it to assertion module.
Add more methods to assertion module. We may even split it to several modules.

I thought we already past that, we had a discussion about that, and decide not to do that, see link above, we going in circles, which is not a good thing.

token-assert would hide those methods, but fix function wouldn't go anywhere it just would be in the different module, if you hide those functions it just would be harder for ppl to contribute to jscs, while assert module would grow, instead modifying the fix method, they would just add more and more stuff to token-assert, which by itself is not good. And yes, plugin writers couldn't easily create their own fixes. Except in the check method, in method, that originally had nothing to do with autofixing.

Otherwise i don't see any difference with those approaches.

I suggest going through the route that already works, if there would be something better, we would do it. Now, i think, we just holding up the progress with such discussions that become resurrected for some reason.

@mrjoelkemp @zxqfox @mikesherov @hzoo thoughts?

[markelog]

Okay, so after long discussion, we came to the compromise, we wouldn't introduce the "fix" field, but a private "fix" field, all support and possible relocation of its logic to the assert module would be on me.

[markelog]

mrjoelkemp added this to the 1.14.0 milestone 25 minutes ago

@mrjoelkemp ?

[mrjoelkemp]

This is 2.0? Thought this was in a good place to land and include in a new release.

Feel free to update the milestone.

[markelog]

Do you want to release 1.14? There is only couple things to do for 2.0, i think i could deal with most of them during this week and release on the next one.

[mrjoelkemp]

We have plenty to fill a 1.14 release; it's been over a month.

To be clear, this particular PR will go into the 2.0 milestone, correct? Feel free to tag any other issues/PRs that should be 2.0 so that I hold off on merging them.

[mrjoelkemp]

This PR seems to be the last one holding up the 1.14 milestone. Are we still aiming to get this in?

[markelog]

This was merged in 2.0, removed the 1.14 tag, also, i would like to propose couple new prs for the new version