RMI-PACTA / pacta.portfolio.report

pacta.portfolio.report
https://rmi-pacta.github.io/pacta.portfolio.report/
MIT License
1 stars 0 forks source link

show highest weight companies, not lowest in company bars #84

Closed cjyetman closed 2 months ago

cjyetman commented 2 months ago

This needs serious review and testing. Just getting a hypothetical fix down for review.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 0.81%. Comparing base (9a24a10) to head (25b8a6a).

Files Patch % Lines
R/prep_key_bars_company.R 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #84 +/- ## ===================================== Coverage 0.81% 0.81% ===================================== Files 25 25 Lines 1596 1596 ===================================== Hits 13 13 Misses 1583 1583 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jdhoffa commented 2 months ago

I triggered a CI/CD build from this PR in https://github.com/RMI-PACTA/workflow.transition.monitor/pull/322 which built the following CH 2024 report: https://pactadatadev.blob.core.windows.net/ghactions-workflow-transition-monitor-results-reports/rmi_pacta_2023q4_pa2024ch-20240611T150226Z/EN/bank/1/working_dir/50_Outputs/rmi_pacta_2023q4_pa2024ch/report/index.html

which has the following company weight plot:

Screenshot 2024-06-11 at 18 09 19

Seems like it worked?

cjyetman commented 2 months ago

Looking at the report that Jackson generated it looks good to me and like it worked. My question is in general what should be the testing procedure for the report from now on? Is checking a report generated by CI/CD and seeing the intended change a proof that what we implemented works?

This particular change seems like an easy one though, if we see it works as it uses a known dplyr function and changes only two lines of code resulting in a behavior that matches expectations. So LGTM.

Not sure there's a "right" answer, but I would assume that, as the maintainer, it's your choice how to test and what passes for adequate testing to merge something.