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

Better error for undialected ColumnExpression #2314

Open
ADBond opened this issue Aug 8, 2024 · 4 comments
Open

Better error for undialected ColumnExpression #2314

ADBond opened this issue Aug 8, 2024 · 4 comments
Labels

Comments

@ADBond
Copy link
Contributor

ADBond commented Aug 8, 2024

If you try to use a ColumnExpression without setting a dialect you get AttributeError: 'ColumnExpression' object has no attribute 'sql_dialect'.

Maybe wrap this in a property that gives a clearer pointer to how to address this.

@browo097302
Copy link
Contributor

Hi I'd be interested in helping with this. What kind of approach did you have in mind? Maybe could have an else statement from the init method to handle when sql_dialect is none with an attribute error showing more clear instructions.
Or as you mentioned wraping in a property, that handles that case?

If you have any further direction on how you would like this done I would be happy to help.

Cheers

@RobinL
Copy link
Member

RobinL commented Oct 7, 2024

if sql_dialect is not None:

I think Andy is suggesting that you use a @property for sql_dialect. This property would raise a more descriptive error when sql_dialect is None, rather than failing silently or setting it directly in the __init__ method.

A typical pattern here would be to store the dialect as self._sql_dialect in the __init__ method. In the property getter, you can check if self._sql_dialect is None, and if it is, raise a more user-friendly error indicating that the dialect needs to be set before usage. This way, you avoid relying on AttributeError and provide a clearer message on how to resolve the issue.

For example:

def __init__(self, sql_expression: str, sql_dialect: SplinkDialect = None):
    self.raw_sql_expression = sql_expression
    self.operations: list[ColumnExpressionOperation] = []
    self._sql_dialect = sql_dialect  # Store dialect as a private attribute

And for the property:

@property
def sql_dialect(self):
    if self._sql_dialect is None:
        raise ValueError("SQL dialect is not set. Please set it before using this ColumnExpression.")
    return self._sql_dialect

We'll have to see whether this approach causes any test failures, but in principle it seems like a sensible approach

@browo097302
Copy link
Contributor

Yeah that makes sense, I will look into that.

@browo097302
Copy link
Contributor

Ah it failed the tests, I did run tests locally and it seemed fine so I need to sort that out on my computer.

It seems to be throwing back an attributeerror: can't set attribute. Maybe it isnt handling the other class methods where sql_dialect is not set? Or perhapbs the sql dialect is not set before use.

Lmk how you think its best to tackle this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants