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

Maintain writer.js or not? #109

Closed
saschanaz opened this issue Nov 3, 2017 · 13 comments
Closed

Maintain writer.js or not? #109

saschanaz opened this issue Nov 3, 2017 · 13 comments
Assignees

Comments

@saschanaz
Copy link
Member

Looking at the git logs, the ws: true option is to enable the writing feature. It will be great to write a whitespace test with the writer (as we can just parse, write, and check the text is intact) but writer.js is currently not being maintained anymore. Should we maintain it or not?

@marcoscaceres
Copy link
Member

Yeah, we should probably maintain it.

@saschanaz
Copy link
Member Author

My original reason to maintain the writer seems circular.

  1. ws: true is for writer.js
  2. writer.js eases test for ws: true
  3. So maintain them?

The writer really seems to be a formatter (as ws: true stores whitespaces selectively), currently I don't have a strong reason to maintain a WebIDL formatter.

@saschanaz
Copy link
Member Author

Creating a WebIDL extension for VSCode with formatting feature may be fun, though 😄

@Zirro
Copy link

Zirro commented Dec 1, 2017

I have a use case where I need to parse IDL, make a few changes to the AST and print it back out with the original formatting intact.

While I'm currently not able to use writer.js for this due to a few problems with the output (in addition to the selective whitespace retention mentioned earlier), a maintained writer.js would be very helpful for this purpose.

@saschanaz
Copy link
Member Author

@Zirro Thank you for sharing your use case! Could I ask you the purpose of making those AST changes?

@Zirro
Copy link

Zirro commented Dec 2, 2017

@saschanaz Sure, it is for an IDL differ/merger to be used while developing jsdom. The tool would make it possible to compare and merge our current IDL files against the latest spec, while ignoring/restoring the changes we have made such as adding custom extended attributes or placing unimplemented properties behind comments.

After doing a few too many manual changes as part of jsdom/jsdom#2053, I hope to fully automate the process the next time.

@saschanaz
Copy link
Member Author

So it seems we should reconsider how ws: true works. @marcoscaceres Thoughts?

@marcoscaceres
Copy link
Member

I'm not fully understanding what @Zirro needs exactly, but I'm happy for us to enhance or change "ws" to accept additional options if need be :)

@saschanaz
Copy link
Member Author

The parser should provide an option to store every whitespace it meets so that one can reconstruct the whole original IDL text with the parsed result. This way one may programmatically modify some IDL types without losing whitespaces.

@marcoscaceres
Copy link
Member

marcoscaceres commented Dec 5, 2017

Oh, I was under the impression it was already doing that. If ws: true doesn't do that, then that's a bug and we should treat it as such.

@saschanaz
Copy link
Member Author

Currently all_ws() without arguments throws away all whitespace tokens it meets. And I think it was on purpose, probably to create a formatter.

@marcoscaceres
Copy link
Member

Will release soon. Travelling atm.

@saschanaz
Copy link
Member Author

It's currently not being exposed for Node package, so probably no need to release this time.

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