-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use recast #110
Comments
I'm thinking of merging recast into master, the only drawback right now is that we won't have ASI fixes. |
RE: estools/escodegen/issues/203 cc @stereokai I've been using recast quite a bit and I think it's good to go to master. I'll be using esformatter as well for formatting. The only thing holding this back at the moment is auto ASI fixes, which should probably be a feature of esformatter. If you'd like to contribute to a project I recommend adding more features to esformatter. |
@goatslacker where can I learn more about these auto ASI fixes? |
I'm referring to having code that's missing semicolons "fixed", or alternatively, have semicolons removed from code that contains them. |
So to get things going, we need to make esformatter be able to perform these to actions? |
Well something has to fix the semicolon issue, either here or esformatter. Probably here since semicolons are not a formatting issue. |
So... where can I start? |
Start thinking of ideas :) The current approach I'm using is to visit nodes based on types, this obviously doesn't work for ASI unless I have a global visitor. Which leads to the next issue: the problem/benefit of using recast is that it only rewrites the parts of the AST that have changed. So if you have semicolons missing everywhere they either won't be added in, or the entire file will be rewritten by recast, which may or may not be a problem if we're using esformatter afterwards. With esformatter we wouldn't actually need recast, but I'd like esformatting to be optional so recast would most likely stay. I don't know, maybe I should just build this and feel it out. |
I'm listening; go on... ;) |
That's all I have right now ^_^ |
Update: I created a v2.0 branch. https://github.com/jshint/fixmyjs/tree/v2.0 Some breaking changes:
I plan to document all of this in Changelog as well as the README. |
Sorry for replying late. Looks like a great start! I would like to comment that I am against moving the config from .jshint to package.json because it diverges from the standard (jshint - which is the fixmyjs "engine") as well as something that is a common practice and would probably be expected by developers. The last point, removal of legacy mode and relying solely on AST is a blessing, and a step in the right direction. Well done! Let's start testing! |
Fair enough, I hate introducing breaking changes but jshint isn't the engine any more so it doesn't make sense to force people to use a For example, if I add new rules in the future I'd have to make sure there is an existing rule on jshint and if there is ever a collision there would be an issue. I feel this is a necessary evil. |
@goatslacker My bad! I thought jshint was doing the hard lifting here, a quick glance leaves no doubt of the contrary. I don't see it as evil at all then, because the benefits listed above stand out now that we're disregarding the jshint "standard'. |
Just forked the |
@goatslacker Now using v2.0 in my environments and all seems fine (I'm Does EDIT: with |
Yeah that needs to be amended if the tool would support v2 |
I noticed on the Dailyjs coverage of this project that it mentions that format styling is lost when running fixmyjs on code. Have you considered using recast? I believe it maintains what styling it can while allowing you to work off the AST for transformations/fixing.
https://www.npmjs.org/package/recast
The text was updated successfully, but these errors were encountered: