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

WIP: Some versioning refactor #1128

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP: Some versioning refactor #1128

wants to merge 1 commit into from

Conversation

SmittieC
Copy link
Collaborator

@SmittieC SmittieC commented Feb 4, 2025

This PR is not meant to be merged, but rather to evoke discussion.

The current versioning code is too complex. Here's some initial code to convey a different approach. Do let me know what you think.

The InstanceDiff would typically be the main entry point, so maybe start there if you want to build a mental model

The main things to focus on is the code in the versioning.py file and the new versioning_fields, get_versioned_field_value and get_versioned_fields_for_display methods.

Basically, we need 1. a way to know which fields to get, 2. we need to get the values for those fields. Sometimes it's not just a simle getattr, but a more comlex query, or in the case of triggers where we want to fetch action params, we need to do that in this step. Lastly, method 3 is for display purposes.

I think here we can do the same as we did with VersionField where we return a single wrapper that knows how to display the current value, but I still need to do that. Anycase, leaving this as-is for now.

@SmittieC SmittieC changed the title WIP: Versioning refactor draft WIP: Versioning refactor Feb 4, 2025
@SmittieC SmittieC changed the title WIP: Versioning refactor WIP: Some versioning refactor Feb 4, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2025

❌ 10 Tests Failed:

Tests completed Failed Passed Skipped
880 10 870 0
View the top 3 failed tests by shortest run time
apps/experiments/tests/test_versioning.py::TestVersion::test_compare
Stack Traces | 0.014s run time
.../experiments/tests/test_versioning.py:76: in test_compare
    version1.compare(similar_version2)
E   AttributeError: 'VersionDetails' object has no attribute 'compare'
apps/experiments/tests/test_versioning.py::TestVersion::test_type_error_raised
Stack Traces | 0.021s run time
.../experiments/tests/test_versioning.py:180: in test_type_error_raised
    version1.compare(version2)
E   AttributeError: 'VersionDetails' object has no attribute 'compare'
apps/experiments/tests/test_versioning.py::TestVersion::test_early_abort
Stack Traces | 0.593s run time
.../experiments/tests/test_versioning.py:97: in test_early_abort
    working_version = experiment.version_details
E   AttributeError: 'Experiment' object has no attribute 'version_details'

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Copy link
Collaborator

@snopoke snopoke left a comment

Choose a reason for hiding this comment

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

@SmittieC I think it might be useful to start with a clean slate rather than try to migrate the existing code (but obviously we can reuse as needed).

Also, I still think the declarative style is the way to go (like django forms). Basic concentp here: https://docs.google.com/document/d/1z09Q9M-mis29DaaD-uuO8myI9Qs6yCM7M0JPCDjQCMA/edit?tab=t.gqi87z9uhxxb

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.

4 participants