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

feat: expose reporter api #41

Merged
merged 4 commits into from
Aug 14, 2017

Conversation

ikatyang
Copy link
Member

@ikatyang ikatyang commented Aug 1, 2017

Fixes #39

@zertosh
Copy link
Member

zertosh commented Aug 2, 2017

Ok, so I had a bit of time to think about this and I'm not sure this is a great approach. Besides, the lack of docs and testing – to make sure this doesn't change w/o breaking you – an API that takes reporter functions creates a larger surface area than necessary.

What is now reportDifferences, should really be a generateDifferences that returns an array of insert/delete/replace ops, then you call your own reporter on them.

I'll put something together later today since it turns out I also kinda need this for a clang-format linter I'm working on.

@ikatyang
Copy link
Member Author

ikatyang commented Aug 2, 2017

I think this package can be separated into three part (eslint-plugin-prettier, generate-differences and show-invisible), all of them are useful and reusable, but it seems you don't want to separate them, so I just refactor them to have minimum changes for exposing API.

maybe we can have something like this?

function generateDifferences(source: string, target: string): Result[];
interface Result {
  operation: Operation;
  offset: number;
  insertText: string;
  deleteText: string;
}
enum Operation {
  Insert,
  Delete,
  Replace,
}

@ikatyang
Copy link
Member Author

ikatyang commented Aug 7, 2017

@zertosh @not-an-aardvark, is this PR okay or what should I do?

@zertosh
Copy link
Member

zertosh commented Aug 7, 2017

I don't have anything constructive to add.

@ikatyang
Copy link
Member Author

Can we get this merged and publish? Thanks. 😅

@not-an-aardvark not-an-aardvark merged commit 1666067 into prettier:master Aug 14, 2017
@ikatyang ikatyang deleted the expose-reporter-api branch August 14, 2017 04:35
@ikatyang
Copy link
Member Author

Is there any schedule for the release including this PR? I'd love to see it happen. ❤️

@not-an-aardvark
Copy link
Member

Oops, I forgot to publish it. I'll do that now.

@not-an-aardvark
Copy link
Member

Published as v2.2.0.

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 this pull request may close these issues.

None yet

3 participants