Skip to content

Revision summary printing #658

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

Merged
merged 16 commits into from
Apr 10, 2025
Merged

Revision summary printing #658

merged 16 commits into from
Apr 10, 2025

Conversation

dajmcdon
Copy link
Contributor

@dajmcdon dajmcdon commented Apr 8, 2025

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • Request a review from one of the current main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION. Always increment
    the patch version number (the third number), unless you are making a
    release PR from dev to main, in which case increment the minor version
    number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    1.7.2, then write your changes under the 1.8 heading).
  • Styling and documentation checks. Make a PR comment with:
    • /document to check the package documentation and fix any issues.
    • /style to check the style and fix any issues.
    • /preview-docs to preview the docs.
    • See Actions GitHub tab to track progress of these commands.
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

@dsweber2 I wanted to improve the printing behavior of revision_summary(). How does this look to you?

@dajmcdon dajmcdon requested a review from dsweber2 April 8, 2025 23:36
Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

Made some fixes, but generally happy to see this.

There were some objects where you were referencing at the wrong depth when printing, which was changing the "No revision" count.

@dajmcdon
Copy link
Contributor Author

dajmcdon commented Apr 9, 2025

Thanks! I want to do one other thing, which is to move the args that apply only to printing to the print method. But I'm having difficulty figuring out what difftime_approx_ceiling_time_delta() is supposed to do (internal/undocumented). Is there a way to avoid that here?

dajmcdon added 2 commits April 9, 2025 10:13
Merge branch 'revision-summary-printing' of https://github.com/cmu-delphi/epiprocess into revision-summary-printing

# Conflicts:
#	R/revision_analysis.R
@dsweber2
Copy link
Contributor

dsweber2 commented Apr 9, 2025

The idea with difftime_approx_ceiling_time_delta is to represent the given duration in the given time_type by rounding up to the nearest whole value of that time_type. For quick_revision on a weekly archive, 3 days rounds up to 1 week instead of being effectively 0 for the purposes of detecting revisions.

@dajmcdon dajmcdon requested a review from dsweber2 April 9, 2025 19:55
@dajmcdon
Copy link
Contributor Author

dajmcdon commented Apr 9, 2025

@dsweber2 Does this still look ok after my changes?

Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

yep, lgtm!

@dajmcdon dajmcdon merged commit 231d979 into dev Apr 10, 2025
3 checks passed
@dajmcdon dajmcdon deleted the revision-summary-printing branch April 10, 2025 18:01
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