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

Resolve logic duplication in the db.types namespace #1100

Closed
dmos62 opened this issue Feb 25, 2022 · 3 comments
Closed

Resolve logic duplication in the db.types namespace #1100

dmos62 opened this issue Feb 25, 2022 · 3 comments
Assignees
Labels
affects: architecture Improvements or additions to architecture affects: technical debt Improves the state of the codebase ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@dmos62
Copy link
Contributor

dmos62 commented Feb 25, 2022

Problem

@mathemancer pointed out that some of the logic in db.types.base::_build_db_types_hinted is a duplicate of logic in db.types.operations.cast (see global variables TEXT_TYPES and NUMBER_TYPES). Specifically it's the logic that describes which types are string-like and which are number-like. This suggests that we could centralize this logic.

Proposed solution

Not totally clear. A new file in the db.types namespace that would hold lists of database types, each list grouping types according to some attribute, like them being string-like or number-like.

A slight complication is that the db.types.base method expects those types to be expressed as Enum instances, while db.types.operations.cast expects them to be type ID strings. You could say that the former wants to convert from Enum to string as late as possible, while the latter as early as possible.

@dmos62 dmos62 added type: enhancement New feature or request affects: architecture Improvements or additions to architecture affects: technical debt Improves the state of the codebase work: backend Related to Python, Django, and simple SQL labels Feb 25, 2022
@dmos62 dmos62 added this to the [07] Initial Data Types milestone Feb 25, 2022
@dmos62 dmos62 self-assigned this Feb 25, 2022
@kgodey kgodey added the ready Ready for implementation label Feb 25, 2022
@dmos62 dmos62 mentioned this issue Feb 28, 2022
7 tasks
@dmos62
Copy link
Contributor Author

dmos62 commented Mar 15, 2022

A slight complication is that the db.types.base method expects those types to be expressed as Enum instances, while db.types.operations.cast expects them to be type ID strings. You could say that the former wants to convert from Enum to string as late as possible, while the latter as early as possible.

@mathemancer I think we could use a convention for how to deal with Enum instance values, or, rather, whether to convert an Enum instance to its value early or late. I've been doing it one way and you another and now we're having interop problems. Maybe a quick sync call at your convenience?

@dmos62
Copy link
Contributor Author

dmos62 commented Mar 18, 2022

This discussion came of the above suggested sync call: #1184

@dmos62 dmos62 closed this as completed May 27, 2022
Repository owner moved this from Ready to Done in [NO LONGER USED] Mathesar work tracker May 27, 2022
@dmos62
Copy link
Contributor Author

dmos62 commented May 27, 2022

Resolved in #1222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: architecture Improvements or additions to architecture affects: technical debt Improves the state of the codebase ready Ready for implementation type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

2 participants