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

Can we simplify TokenStreamRewriter's overloads? #163

Open
BurtHarris opened this issue Oct 23, 2016 · 1 comment
Open

Can we simplify TokenStreamRewriter's overloads? #163

BurtHarris opened this issue Oct 23, 2016 · 1 comment
Assignees
Labels

Comments

@BurtHarris
Copy link
Collaborator

BurtHarris commented Oct 23, 2016

The set of overloads in TokenStreamRewriter.tssoon` is seems over-complicated. I'm wondering if a little refactoring is in order, at least for the TypeScript port.

  1. The optional programName argument at the beginning of insertAfter, insertBefore, replace and delete methods seems unnecessary. Instead, clients using this API could create separate instances of TokenStreamRewriter, each one serving as it's own "program".

  2. The token index are overloaded with both number and Token type seem like they could be replaced with number | Token types, further reducing the number of overloads.

  3. Where the API includes a text argument, is there any reason this shouldn't be typed string

Thus I'd suggest the side-effecting methods be reduced to:

insertAfter(index: number|Token, text: string): void;
insertBefore(index: number|Token, text: string): void;
replace(index: number|Token, text: string): void ;
replace(from: number|Token, to: number|Token, text: string): void ;
delete(from: number|Token, to?: number|Token): void;

This seems like a pretty complete set of operations, but the class as currently defined seems to support sub-classing it, which exposes substantially more surface area (e.g. the nested operation classes). Was a useful application for sub-classing TokenStreamRewriter discovered in Java?

@BurtHarris
Copy link
Collaborator Author

Years later, answering my own question. Yes, I'm convinced the overloads are wrong for TypeScript when optional parameters can be used. and TokenStreamRewriter is not the only case of this. I believe this calls for a review of the whole codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants