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.

Sign up for GitHub

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

Jump to bottom

ComparisonLevel composition v2 #1114

Merged
merged 7 commits into from
Mar 29, 2023
Merged

ComparisonLevel composition v2 #1114

merged 7 commits into from
Mar 29, 2023

Conversation

NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Mar 11, 2023 •
edited
Loading

This is a spin-off of #1096. Changes from that PR

  • Rebased to on top of current master
  • Changed the shared testing stuff to be a simple function instead
    of a fixture, since it's not expensive to construct
    and we don't need multiple versions per test
  • made ComparisonLevel.label_for_charts public as well
  • Used ComparisonLevel objects in the composition logic
    instead of raw dicts
  • Removed all the complex "label_for_charts" generating stuff. That is
    the responsibility of the ComparisonLevel and should be
    done there. We can port over the nice SQL normalization
    stuff that @ThomasHepworth did, but that should be in a separate
    follow up PR, and more central to the ComparisonLevel,
    not specific to this composition stuff.

@NickCrews
docs: fix typos in settings_dict_guide.md
500f305
@NickCrews
fmt: Black everything
f459aaf 
Some things seemed to not have been conforming to black.
@NickCrews
docs: fix typo in jsonschema.json
05c8c35
@NickCrews
ref: add .is_null_level and .sql_condition to ComparisonLevel
5792352
@NickCrews
ref: make ComparisonLevel.label_for_charts public
1e40828 
It is used by these other classes so it really is public.
And it is a public part of the API once you convert the
ComparisonLevel to a dict, so it really is public.
@NickCrews
test: import common test utils as if they are siblings in package
b8e381e 
They were getting treated like these common testing modules
were 3rd party, but they are actually 1st party
@NickCrews
feat: Add ComparisonLevel compositions
65ded60 
This is a spin-off of
moj-analytical-services#1096
@NickCrews NickCrews mentioned this pull request Mar 11, 2023
Closed
@ThomasHepworth ThomasHepworth changed the base branch from master to comparison_level_composition March 28, 2023 12:38
@ThomasHepworth ThomasHepworth changed the base branch from comparison_level_composition to master March 28, 2023 12:59
ThomasHepworth
ThomasHepworth approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@ThomasHepworth ThomasHepworth left a comment

Choose a reason for hiding this comment

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

Thanks Nick, your changes all look sensible.

I'm happy to omit the comparison level chart names code.

@NickCrews
Copy link
Contributor Author

Sounds good, thanks for the review! Let me know if there's anything I can do to usher this along.

@ThomasHepworth ThomasHepworth merged commit fb48f23 into moj-analytical-services:master Mar 29, 2023
@RossKen RossKen mentioned this pull request Mar 30, 2023
Merged
@ADBond ADBond mentioned this pull request Apr 4, 2023
Closed
@ThomasHepworth ThomasHepworth mentioned this pull request Jul 10, 2023
Merged
9 tasks
@NickCrews NickCrews deleted the cll_comp_v2 branch February 14, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomasHepworth ThomasHepworth ThomasHepworth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NickCrews @ThomasHepworth