Skip to content

Conversation

kingbuzzman
Copy link
Contributor

No description provided.

@kingbuzzman kingbuzzman force-pushed the dev/freezer-cli-migrate branch from 569bc85 to b895333 Compare August 25, 2025 13:18
Comment on lines +120 to +124
# Phase 1: Collect all information
state = collect_migration_state(ast_obj)

# Phase 2: Generate callbacks based on collected state
callbacks = generate_callbacks(state)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this two-pass approach, it decouples things too much

it's better to have the token-rewriting functions read bits of state as necessary

see examples in django-upgrade such as

https://github.com/adamchainz/django-upgrade/blob/37f31275abfe9b9d6e56627adbfb1a9ac12674ae/src/django_upgrade/fixers/admin_register.py#L25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue is that i needed to see if there is both freezegun and freezer -- and if there is, just go with freezer. I'll rethink the strategy. thanks for the feedback

Comment on lines +25 to +27
@dataclass
class State:
"""State collected during preprocessing to guide transformations."""
Copy link
Owner

Choose a reason for hiding this comment

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

please don't use a giant dataclass of state, I would prefer some local variables, which may use mini classes to collect several fields. Use plain classes, not dataclasses, for speed.

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.

2 participants