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: Refactor the output comparator to consider dropped errors #1238

Merged
merged 10 commits into from
Aug 24, 2022

Conversation

bdferris-v2
Copy link
Collaborator

Also includes a general refactoring of the report generation logic:

  • pull more logic out of Main
  • adding specific model classes for JSON serialization

As discussed in #1232.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues

… a general refactoring of the report generation logic. Includes pulling more logic out of Main and adding specific model classes for JSON serialization.
@bdferris-v2 bdferris-v2 linked an issue Aug 15, 2022 that may be closed by this pull request
@bdferris-v2
Copy link
Collaborator Author

@maximearmstrong this is mostly ready for review and I'd love your feedback if you have cycles.

It isn't fully ready to review/commit because I haven't updated the comment_generator.py script yet. That said, I wanted to propose a secondary refactor there as well. The outputcomparator is already producing a block of text to be logged by the output comparator that seems pretty similar to what's being generated by the comment_generator.py script. Maybe we could have outputcomparator just save its log lines to a file and use that in the PR comment, scraping the script entirely?

@maximearmstrong
Copy link
Contributor

Maybe we could have output comparator just save its log lines to a file and use that in the PR comment, scraping the script entirely?

I agree. I think it would also be better from a design standpoint if the output comparator took on that responsibility.

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

I've added a few comments, but overall it's great! The new design brings a lot of clarity to the code. Thanks a lot @bdferris-v2!

@bdferris-v2 bdferris-v2 marked this pull request as ready for review August 17, 2022 21:42
@bdferris-v2
Copy link
Collaborator Author

I've updated the comment generation code, as discussed. This is ready for final review.

@isabelle-dr
Copy link
Contributor

Thanks for working on this @bdferris-v2 !
I have added a comment above. We might need to update ACCEPTANCE_TESTS.md to reflect the changes in this PR.

What is going to be the schema of the new report with these changes?

@maximearmstrong
Copy link
Contributor

I've approved the PR. Thanks @bdferris-v2!

We might need to update ACCEPTANCE_TESTS.md to reflect the changes in this PR.

What is going to be the schema of the new report with these changes?

@isabelle-dr do we need clarity on these items before merging?

@isabelle-dr
Copy link
Contributor

Thanks Maxime, I think it is in the scope of this PR to update ACCEPTANCE_TESTS.md with the changes (addition of dropped errors, report schema change).

@bdferris-v2
Copy link
Collaborator Author

@isabelle-dr I updated the ACCEPTANCE_TESTS.md to reflect the changes in this PR.

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks @bdferris-v2

@maximearmstrong maximearmstrong merged commit 8d8a478 into MobilityData:master Aug 24, 2022
@bdferris-v2 bdferris-v2 deleted the issue/1232/comparator branch October 7, 2022 18:44
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.

Improve acceptance rules
3 participants