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: Custom reporters support #1643

Merged
merged 31 commits into from
Sep 13, 2021

Conversation

mad-gooze
Copy link
Contributor

@mad-gooze mad-gooze commented Sep 6, 2021

Hi Jason!

This PR proposes custom reporters support which allows implementing things like #21 in the future.

It definitly needs better documentation and tests, could you review it and discuss if you find the way I implemented is ok?

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 6, 2021

Cool. I'll take a look at it.

@mad-gooze mad-gooze changed the title adds support for custom reporters Сustom reporters support Sep 6, 2021
@mad-gooze mad-gooze marked this pull request as draft September 6, 2021 11:17
@Jason3S
Copy link
Collaborator

Jason3S commented Sep 6, 2021

I'm impressed. You put a lot of work into it.

I like the idea of having multiple reporters. Then it can work with custom loggers.

packages/cspell-json-reporter/cSpell.js Outdated Show resolved Hide resolved
packages/cspell-json-reporter/package.json Outdated Show resolved Hide resolved
packages/cspell/src/util/reporters.ts Outdated Show resolved Hide resolved
packages/cspell/src/util/reporters.ts Outdated Show resolved Hide resolved
test-packages/test-cspell/src/index.ts Show resolved Hide resolved
@Jason3S
Copy link
Collaborator

Jason3S commented Sep 7, 2021

Thank you for all the effort!

@mad-gooze
Copy link
Contributor Author

Thanks for CR, I'll try to fix the issues this week.

What do you think of additional testing? Do I need to add any unit tests before this can be merged?

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 8, 2021

What do you think of additional testing? Do I need to add any unit tests before this can be merged?

It took a lot of effort to get the code coverage as high as it is. I would like to keep it that way.

If you want some help with tests, I can help out by breaking out some of the independent stuff and getting it landed.

Copy link
Collaborator

@Jason3S Jason3S left a comment

Choose a reason for hiding this comment

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

Almost there!

@@ -0,0 +1,42 @@
{
"issues": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a sample file? Or a result of running a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this a result of running example, I've forgotten to put in in gitignore

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, it is best to write it into temp, some of the other packages do the same thing.

Copy link
Collaborator

@Jason3S Jason3S left a comment

Choose a reason for hiding this comment

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

Almost there. Would you like some help with the last bit?

@mad-gooze mad-gooze marked this pull request as ready for review September 12, 2021 23:20
@mad-gooze
Copy link
Contributor Author

I think it's complete?

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 13, 2021

Great! I'll land it.

@Jason3S Jason3S merged commit 3b9ac1b into streetsidesoftware:main Sep 13, 2021
@Jason3S Jason3S changed the title Сustom reporters support feat: Custom reporters support Sep 16, 2021
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.

2 participants