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

Align on facet variable naming convention #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nathanielw44
Copy link

@nathanielw44 nathanielw44 commented Nov 25, 2024

Overview

  • Align on common naming convention for facet group vs facet dict vs facet name instead of just calling blanket calling everything "facet" and getting confused.
  • Rename a bunch of variables to be more explicit about what type they actually are
# A "facet group" is a list of facet dicts that are all applied together with an AND or OR type 
facet_group = {
	'type': 'or',
	'facets': [
		{'key': 'feature_needed', 'value': '' },
		{'key': 'feature_needed', 'value': 'metric' },
		...
	]
}

# a facet dict is one of the key value pairs that a of data object can match 
facet_dict = {'key': 'feature_needed', 'value': 'metric' }

# The "key" of a facet dict is a "facet_name"
facet_name = 'feature_needed'

How to test

  • Let's see if the build passes
  • Naming changes make the code more clear and easier to read than before

@nathanielw44 nathanielw44 changed the title rename a bunch of variable Align on facet variable naming convention Nov 26, 2024
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 12033921404

Details

  • 63 of 63 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 92.446%

Totals Coverage Status
Change from base Build 7672290461: 0.008%
Covered Lines: 820
Relevant Lines: 887

💛 - Coveralls


norm_terms_updated = norm_terms != old_norm_terms
facets_updated = facets != old_facets
facets_updated = facet_names != old_facet_dicts
Copy link
Author

@nathanielw44 nathanielw44 Nov 26, 2024

Choose a reason for hiding this comment

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

@anthonyp97 This is the bug you fixed you fixed in #38 - comparing facet_names with facet dicts. Decided to leave it alone for now because fixing it breaks a ton of tests, so I think it warrants its own PR

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