Skip to content
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

Reformat the output somehow #35

Closed
domenic opened this issue Feb 8, 2017 · 8 comments
Closed

Reformat the output somehow #35

domenic opened this issue Feb 8, 2017 · 8 comments
Assignees

Comments

@domenic
Copy link
Member

domenic commented Feb 8, 2017

The indentation is already not perfect, and #33 is going to add more imperfect indentation.

It would be ideal if while writing webidl2js, we could just ignore the indentation issues (or use whichever looks nicer inside the webidl2js source code), then have a postprocessing step that reformats everything for us. Maybe eslint's fix option, or maybe something simpler based on just esprima.

@TimothyGu
Copy link
Member

A more radical approach is we can just rewrite the entire code generator based on Babel or escodegen -- that is, generating an AST instead of JS code directly. Something like babel-template makes it less painful than it would appear at first.

@Sebmaster
Copy link
Member

Oh yes. babel-template looks awesome. I originally wanted to avoid building ASTs due to the verbosity, but this might just be perfect.

@domenic
Copy link
Member Author

domenic commented Feb 8, 2017

It's worth noting that the original domenic/webidl-class-generator did something similar using Traceur's babel-template counterpart. See e.g. https://github.com/domenic/webidl-class-generator/blob/master/lib/code-generation.js

@domenic
Copy link
Member Author

domenic commented Feb 12, 2017

For direct source-to-source prettification, https://github.com/jlongster/prettier is the new hotness.

@domenic domenic self-assigned this May 6, 2017
domenic added a commit that referenced this issue May 6, 2017
@domenic
Copy link
Member Author

domenic commented May 6, 2017

OK, so I did the prettier part of this. That was easy.

Remaining tasks until I personally am happy:

  • Reformat all multiline code strings in the source to be indented in a way that is pleasing to read and write, instead of concerned with what the output would look like. (E.g., have leading whitespace indentation as if they were code and so on.)
  • Add an eslint config to this project, probably similar to jsdom's, and fix all issues with this project.
  • Change from prettier to prettier-eslint to get even more cleanups and fixups, using the project's ESLint config.

But I want to delay these until landing #33 as I don't want to make @TimothyGu rebase unnecessarily on any lint fixes and reformatting. So this is on hold for a little bit.

@TimothyGu
Copy link
Member

Change from prettier to prettier-eslint to get even more cleanups and fixups

I'm not sure if we want to put ESLint in webidl2js's dependencies, which is necessary for prettier-eslint to work. ESLint is much bigger than prettier, while prettier has 0 dependencies (since it's bundled by rollup).

@Sebmaster
Copy link
Member

I'm not sure if we want to put ESLint in webidl2js's dependencies, which is necessary for prettier-eslint to work.

Since webidl2js is mostly a devDependencies tool in general itself, I don't think size considerations matter much. It's not used at runtime anyways.

TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Jul 31, 2017
Also add a bunch of tests.

On the road to fixing jsdom#35.
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Jul 31, 2017
On the road to fixing jsdom#35.
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Aug 2, 2017
Also add a bunch of tests.

On the road to fixing jsdom#35.
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Aug 2, 2017
On the road to fixing jsdom#35.
@TimothyGu TimothyGu mentioned this issue Aug 2, 2017
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Aug 2, 2017
Also add a bunch of tests.

On the road to fixing jsdom#35.
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Aug 2, 2017
On the road to fixing jsdom#35.
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Aug 2, 2017
On the road to fixing jsdom#35.
TimothyGu added a commit to TimothyGu/webidl2js that referenced this issue Aug 2, 2017
On the road to fixing jsdom#35.
TimothyGu added a commit that referenced this issue Aug 2, 2017
On the road to fixing #35.
TimothyGu added a commit that referenced this issue Aug 2, 2017
On the road to fixing #35.
@domenic
Copy link
Member Author

domenic commented Aug 19, 2017

I guess this is indeed good enough as-is.

@domenic domenic closed this as completed Aug 19, 2017
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

No branches or pull requests

3 participants