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

Expose report-related APIs in another package #39

Closed
ikatyang opened this issue Jul 31, 2017 · 5 comments · Fixed by #553
Closed

Expose report-related APIs in another package #39

ikatyang opened this issue Jul 31, 2017 · 5 comments · Fixed by #553

Comments

@ikatyang
Copy link
Member

Currently, all the APIs are not exposed, so that it can't be reused by other linter-plugin, e.g. tslint-plugin-prettier (currently just copy-paste implementations from this repo).

It'd be better to expose report-related APIs in another package (since this repo has peerDeps for eslint), so that it can be reused in different linter plugin to have consistent behavior.

Suggested API

export declare function showInvisibles(str: string): string;
export declare function reportDifferences(source: string, formatted: string, reporters: Reporters);

export interface Reporters {
  reportInsert: (offset: number, text: string, message: string) => void;
  reportDelete: (offset: number, text: string, message: string) => void;
  reportReplace: (offset: number, deleteText: string, insertText: string, message: string) => void;
}
@not-an-aardvark
Copy link
Member

I wouldn't mind doing this in theory, but it feels like the helper functions are sufficiently small and eslint-specific that it wouldn't be worthwhile. For example, all of the helpers use getLocFromIndex, context, and fixer, which are ESLint APIs and (as far as I know) have no direct equivalent in tslint.

I don't think your proposed interface would work, because something like reportInsert would need to have some way of actually reporting an error, and that can't happen unless they either return a value or accept a report function.

@ikatyang
Copy link
Member Author

ikatyang commented Jul 31, 2017

The context is not a necessary argument for reporters, for example if there is the suggested API:

return {
  Program() {
    if (!prettier) {
      // Prettier is expensive to load, so only load it if needed.
      prettier = require('prettier');
    }
    const prettierSource = prettier.format(source, prettierOptions);
    if (source !== prettierSource) {
      suggestedAPI.reportDifferences(source, prettierSource, {
        reportInsert: (...args) => reportInsert(context, ...args),
        reportDelete: (...args) => reportDelete(context, ...args),
        reportReplace: (...args) => reportReplace(context, ...args),
      });
    }
  }
};

We can call context as a contextual value so that it wouldn't have to pass in reporters directly.

For TSLint vs ESLint API, see this.

If you still think my proposed interface wouldn't work, I can send a PR here to show what it'd be in my proposal.

@ikatyang
Copy link
Member Author

See #40 for my proposal

@not-an-aardvark
Copy link
Member

cc @zertosh, any thoughts on this?

@zertosh
Copy link
Member

zertosh commented Jul 31, 2017

Sounds good to me!

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

Successfully merging a pull request may close this issue.

3 participants