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

fix(cli): do not return duplicate results #681

Merged
merged 5 commits into from
Nov 10, 2019
Merged

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Oct 14, 2019

Fixes #680

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@P0lip P0lip added the t/bug Something isn't working label Oct 14, 2019
@P0lip P0lip self-assigned this Oct 14, 2019
const ARTIFICIAL_ROOT = Symbol('root');

const serializeRange = ({ start, end }: IRange) => `${start.line}:${start.character}:${end.line}:${end.character}`;
const getIdentifier = (result: IRuleResult) => `${result.path.join('/')}${result.code}${serializeRange(result.range)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P0lip Just an idea. How about turning that into a comparisonFunction (cf. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort)?

It could be used to detect duplicates in that use case (when === 0) and reused in stylish to order the results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My sincere apologies for dropping a ball on this one.
Your new WIP PR reminded me of this PR.
Seems like you've started some work on that, so how do you feel about merging this one and implementing the comparison fn in your PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you've started some work on that

Well... I was merely scratching an itch to ensure my proposal was actually bringing some kind of value. 😉

so how do you feel about merging this one and implementing the comparison fn in your PR?

@P0lip 👍

@P0lip P0lip requested a review from marbemac November 5, 2019 11:34
@P0lip P0lip merged commit 35e85b4 into develop Nov 10, 2019
@P0lip P0lip deleted the fix/duplicate-results branch November 10, 2019 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplication of results
3 participants