Add overall column, clean up full stops and clean_data functions

AJFOWLER commented 4 years ago

PR Checklist

All Submissions:

[ x ] Have you followed the guidelines in our Contributing document?
[ x ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
[ x ] Does your submission pass R CMD check locally?

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Three changes:

Final 'overall' column labelled in line with uploaded data - fixes issue #96
Add full stops to the end of all domain labels (ROBx and others) - fixes issue #95
Abstracted some of the data cleaning functionality.

What is the current behavior? (You can also link to an open issue here)

Issue #96: Overall column was hardcoded,
Issue #95: Full stops were omitted from the end of some domain labels
Data cleaning was iterative loops that were repeated for all traffic lights

What is the new behavior (if this is a feature change)?

Issue #96: Overall column now is just named after the final column of uploaded data (after weights excluded)
Issue #95: Added full stops where missing and generated new vdiffr test cases.
Data cleaning is now mostly done in a single function clean_data() - this could probably be extended to the rob_summary functions too.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

Other relevant information:

Thank you for taking the time to submit this PR!

mcguinlu commented 4 years ago

Hi @AJFOWLER,

This all looks great - and a lot cleaner than before! I always get pangs of shame looking back at the inital knocked-together versions of some of the package's functions 😨

Some small follow-up things:

[ ] Can you run styler::style_pkg() and commit the changes? It will just make it easier for me to find and address true merge conflicts, rather than those that are just due to styling.
[ ] We'll need to add tests for the new functionality you created for the "Generic" template - I've documented the tests required in a seperate issue for reference (#100). I appreciate that you might not have the bandwith for this at present - if not, just let me know, and I'll add them at a later point.
[ ] Finally, if you are interested in doing so, please add yourself as a contributor [ctb] in the DESCRIPTION file (for info, see Section 8.3 here). Your input is definitely helping to improve robvis, particularly in making it much easier to develop new functionality, so I think it is only right that this is formally acknowledged.

A last aside: as per your comment in #97, I completely agree that a lot of the code for actually producing the graphs is repeated and could be refactored. Heck, even creating a function that returns the ggplot2 elements common to all plots would be a great start. However, I am keen to get this PR merged into the main branch soon, as robvis is participating in the Hacktoberfest event, and so I'd like to keep the main branch as up to date as possible so anyone taking a fork is taking the most current version. So I think the best option is to finish this PR, then deal with the common plotting code!

AJFOWLER commented 4 years ago

Hi @mcguinlu

In these commits:

Tests added as requested in #100
styler::style_pkg() run and committed.
While I was in the rob_traffic_lights etc I sorted the error (that I'm fairly sure I caused!) mentioned in #99 , I've added a test for that too.
I fixed #97.
I added myself as a collaborator, thanks :).

Agree with refactoring the common plotting code being something to defer 👍

mcguinlu commented 4 years ago

All looks great - thanks for getting around to it so quickly! There are few merge conflicts which I am going to address in the morning with a clear head, and then I'll pull in your changes.

mcguinlu commented 3 years ago

Due to me making a Hames of resolving the merge conflicts, this is now being dealt with in #101 - sorry for the delay in doing this @AJFOWLER!

Happy Halloween 🎃

mcguinlu / robvis