Skip to content

Refactoring/#294 refactor column name type #295

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

ckunki
Copy link
Contributor

@ckunki ckunki commented May 14, 2025

Closes #294
Closes #292

@ckunki ckunki had a problem deploying to manual-approval May 14, 2025 15:24 — with GitHub Actions Failure
@ckunki ckunki had a problem deploying to manual-approval May 14, 2025 15:35 — with GitHub Actions Failure
@ckunki ckunki temporarily deployed to manual-approval May 14, 2025 15:37 — with GitHub Actions Inactive
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diff is quite large for this file.

@ckunki ckunki temporarily deployed to manual-approval May 14, 2025 15:49 — with GitHub Actions Inactive
replaced by property .rendered
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 06:22 — with GitHub Actions Failure
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 06:25 — with GitHub Actions Failure
@ckunki ckunki temporarily deployed to manual-approval May 15, 2025 06:25 — with GitHub Actions Inactive
@ahsimb
Copy link
Contributor

ahsimb commented May 15, 2025

A general comment. The column.py is now mostly about the column type. Would you like to create a separate file column_type.py and move the type classes there? Also, I think naming should change. For example the VarcharColumn should be renamed to VarcharColumnType or VarcharType.

@ckunki
Copy link
Contributor Author

ckunki commented May 15, 2025

A general comment. The column.py is now mostly about the column type. Would you like to create a separate file column_type.py and move the type classes there? Also, I think naming should change. For example the VarcharColumn should be renamed to VarcharColumnType or VarcharType.

Thanks for this comment.

I needed to put class ColumnType and subclasses and top level class Column into one file to calm down mypy.

Reason is that each subclass still (and again) contains a class method simple() which now does not create an instance of a ColumnType but of Column. I find this convenient from a user's perspective. But I agree that this might not be 100% clean from a typing perspective.

This also is a reason for not naming the subclasses VarCharColumnType or VarCharType but VarCharColumn, see the examples in the user guide which read nicely, I hope.

However if you prefer then I can also replace methods simple() from the subclasses of ColumnType by top-level functions, eg. varchar_column().

This would then enable

  • splitting up the file into column.py and column_type.py
  • renaming the subclasses of ColumnType to VarCharColumnType or VarCharType

@ahsimb
Copy link
Contributor

ahsimb commented May 15, 2025

I didn't notice first that the simple still creates a Column. It looks really weird when A contains B, but B can create A. I would strongly suggest that the column type minds its own business. So for example it can theoretically be used in something like MyFancyColumn which is not derived from Column.



@dataclass(frozen=True, repr=True, eq=True)
class BooleanColumn(Column):
class BooleanColumn(ColumnType):
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the super type is ColumType, I am not sure if BooleanColumn is an appropriate name , some for the other ones

@@ -31,7 +27,7 @@ def __get__(self, owner_self, owner_cls):


@dataclass(frozen=True, repr=True, eq=True)
class Column:
class ColumnType:
Copy link
Collaborator

Choose a reason for hiding this comment

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

would SQLType maybe more correct?

Copy link
Contributor Author

@ckunki ckunki May 15, 2025

Choose a reason for hiding this comment

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

Not too bad in my eyes. I only already have a class SqlType in column_type_utils.py already, that should be renamed then.

Replaced method simple() of subclasses of ColumnType by top-level functions
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 13:36 — with GitHub Actions Failure
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 13:41 — with GitHub Actions Failure
return self.fget(owner_cls)


@dataclass(frozen=True, repr=True, eq=True)
Copy link
Contributor Author

@ckunki ckunki May 15, 2025

Choose a reason for hiding this comment

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

Compared to the last commit, the classes in this file have been moved from column.py.

@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 13:44 — with GitHub Actions Failure
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 14:09 — with GitHub Actions Failure
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 14:32 — with GitHub Actions Failure
@ckunki ckunki had a problem deploying to manual-approval May 15, 2025 14:32 — with GitHub Actions Failure
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.

Refactor class Column once again Added check for range of size attribute to HashTypeColumn
3 participants