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

Draft: Update reporters #1

Merged
merged 3 commits into from
May 5, 2021

Conversation

aarongoldenthal
Copy link

@aarongoldenthal aarongoldenthal commented May 5, 2021

@dwightjack

This is a draft, but wanted to put it out to get your thoughts.

I'm not sure if GitHub will make this work, but I thought trying to PR my changes to your fork's branch could then carry over into your PR to pa11y.

This includes the following updates:

  • Add a JSON reporter. With no options is writes JSON to stdout. It can also accept configuration with a fileName property to write directly to a file.
  • Update resolver logic to accept the shorthand cli and json and resolve the appropriate included reporters, which would allow them to be selected without having to include the local path. I included the specific resolved names to allow names down the line that did not match the shorthand. The other option I considered was updating loader to try to resolve in the lib/reporters/ folder first, but the login seemed overly complicated.
  • Updated readme with the JSON reporter details and configuration.

I was planning to put in a test for multiple reporters, but ran out of time and wanted to get this much out there.

I did not address the --json CLI option at all, was hoping to head some input on that from the pa11y team. My vote is still to just remove it for consistency, but if not rather than leaving the duplicate logic I'd propose setting the reporter to json if that option is selected, overriding --reporter (another reason to just remove it).

@aarongoldenthal aarongoldenthal changed the title DraftAdd reporters Draft: Update reporters May 5, 2021
@dwightjack dwightjack self-assigned this May 5, 2021
@dwightjack dwightjack self-requested a review May 5, 2021 08:20
@dwightjack
Copy link
Owner

@aarongoldenthal That's great! Thanks!
Sorry for not working on this lately. I took some days off 😅.

The implementation looks great. Let's merge this PR (I think it should update my PR as well). I am going to add those missing tests. By the way, I've never touched the integration tests. Do you think those tests related to multiple reporters should be implemented there?

About the --json flag, I think we could follow a more conservative approach by deprecating it (ie: adding some warning) and implement it as you suggest (when set, reporters config is ignored and 'json' reporter is used instead).

@dwightjack dwightjack merged commit d4da4fb into dwightjack:master May 5, 2021
@dwightjack dwightjack removed their request for review May 5, 2021 08:33
@aarongoldenthal
Copy link
Author

@dwightjack No problem, we all need days off :)

Thinking about it a little more and looking through the tests again I could see the multiple reporter test in either or both places honestly. If it's unit, it seems like it would fit pretty cleanly with the other reporter tests. The integration tests are all pretty straightforward, they just spawn a child process with the CLI command using the appropriate config file and check the console output. The key is probably picking the best reporter to check the two results conclusively.

@aarongoldenthal
Copy link
Author

And I probably won't have a chance to look again tonight, so before I forget I was thinking this would probably be a logical place to add the --json logic setting the JSON reporter, this logic could be removed since handled by the reporter (that's where I started from), and if I'm reading it right the JSON integration test would still pass as-is.

@aarongoldenthal aarongoldenthal deleted the add-reporters branch December 10, 2021 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants