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

Enhancement: Easier switching between summary and details #52

Merged

Conversation

christineberger
Copy link
Contributor

Description & motivation

Issue #33 Enhancement

  • Changed compare_queries() macros to take additional argument with param summary, which is set to true by default.

  • Changed compare_queries() SQL to switch between summary and details SQL dependent on summary value

  • Changed compare_relations() macros to take additional argument with param summary, which is set to true by default.

  • Ensured summary argument is passed in to the call to compare_queries() macro within the compare_relations() macro.

  • Added tests and test data for both compare_queries and compare_relations macros with and without including the summary argument.

  • Added details on the summary parameter in the README.

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@christineberger christineberger changed the title Enhancement/easier switching between summary and details Enhancement: Easier switching between summary and details Aug 9, 2022
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

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

Love this! Two tweaks and then let's get it merged 🎉 @leoebfolsom, just checking that this doesn't clash with anything you've got in flight? I know you've been poking around these parts recently too

README.md Outdated Show resolved Hide resolved
@leoebfolsom
Copy link
Contributor

leoebfolsom commented Aug 11, 2022

Love this! Two tweaks and then let's get it merged 🎉 @leoebfolsom, just checking that this doesn't clash with anything you've got in flight? I know you've been poking around these parts recently too

@joellabes Somewhat to my surprise since this PR is indeed in a similar spirit as mine, there don't seem to be any direct conflicts; and if I've somehow missed that, I'll be happy to incorporate these changes into mine. So, no need to worry about my PR before this one is merged. Thanks!

@christineberger
Copy link
Contributor Author

christineberger commented Aug 12, 2022

@joellabes Everything's changed and updated!

Edit:
I used summarize 🤣 Only because of this definition from Google:

Summarise is more common in British English, where summarize can also be found frequently. Summarize is more common in American English, where summarise is rarely used.

Decided to take the most commonly found.

@joellabes
Copy link
Contributor

🎉 big fan of this
image

@joellabes joellabes merged commit dd5d2ed into main Aug 12, 2022
@joellabes joellabes deleted the enhancement/easier_switching_between_summary_and_details branch August 12, 2022 19:49
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.

3 participants