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

Suggestions for the count content values transformation #83

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

mephenor
Copy link
Member

@mephenor mephenor commented Nov 7, 2024

Mostly renamed some things and switched some stuff around.

The content prefix was a bit confusing, because it seemed like that should refer to entities in what's denoted by class_name, but that was already taken by the target prefix here...

  • target prefix should be as before, but renamed everything that is pointed to by the relation path to have a referenced prefix.
  • Factored the content of the outermost loop out into its own function and removed context class to reduce clutter a bit.
  • Changed function signatures to require kwargs and only pass around, what's really required by creating a few intermediate variables. Should hopefully be easier on the eyes this way
  • Renamed some functions and adjusted docstrings to be less generic

Hope that fits the intended purpose

@mephenor mephenor requested a review from sbilge November 7, 2024 18:45
@coveralls
Copy link

Pull Request Test Coverage Report for Build 11731960176

Details

  • 33 of 36 (91.67%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.007%) to 93.124%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/metldata/builtin_transformations/count_content_values/data_transform.py 33 36 91.67%
Files with Coverage Reduction New Missed Lines %
src/metldata/builtin_transformations/count_content_values/data_transform.py 1 90.0%
Totals Coverage Status
Change from base Build 11722640716: -0.007%
Covered Lines: 921
Relevant Lines: 989

💛 - Coveralls


return target_object
target = resolve_data_object_path(data=data, path=path)
Copy link
Member

Choose a reason for hiding this comment

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

In add_content_properties -> data_transform.py transformation, the same fn call is assigned to the variable name object which is not super explanatory. It would be good to change that too.

Copy link
Member

Choose a reason for hiding this comment

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

But I can do it in the main PR.

@sbilge sbilge merged commit 3df42e6 into count_content_values Nov 8, 2024
4 checks passed
@sbilge sbilge deleted the count_content_values_suggestions branch November 8, 2024 10:04
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