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

implement a janitorclass to automatically format class attributes #1650

Closed
wants to merge 1 commit into from

Conversation

ThomasHepworth
Copy link
Contributor

Type of PR

  • BUG
  • FEAT
  • MAINT
  • DOC

Is your Pull Request linked to an existing Issue or Pull Request?

Follows on from #1642.

Give a brief description for the solution you have provided

Adds in a simple JanitorClass that automatically sets class attributes to upper case. This streamlines some of the SQLglot code and removes the need for a verbose try except clause to grab our quote identifiers.

If you think this is over-engineering the solution, then I shall scrap it!

Just to note, this can also be done with metaclasses with some inheritance magic. However, as SQLglot already makes use of metaclasses, I've just implemented a wrapper class that manipulates how attributes are pulled.

PR Checklist

  • Added documentation for changes
  • Added feature to example notebooks or tutorial (if appropriate)
  • Added tests (if appropriate)
  • Made changes based off the latest version of Splink
  • Run the linter

@RobinL RobinL self-assigned this Oct 16, 2023
Copy link
Member

@RobinL RobinL left a comment

Choose a reason for hiding this comment

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

This is clever, but I guess maybe this is my lack of Python knowlege but it took me a little while to understand what this is doing.

On that basis, my view is:

  • If implementing the JanitorClass (possibly better named UpperCaseAttributeWrapper or similar) was going to save us loads of lines of code elsewhere, then it's worth the complexity
  • If (as it looks like) it saves only a small amount of conditionals in our code, then it might not be worth the complexity, especially since if we wait a bit, we can make sqlglot>16 a requirement, and then the class is no longer needed

@RobinL
Copy link
Member

RobinL commented Oct 16, 2023

Have discussed with Tom - going to close this one because it possibly introduced a bit too much indirection

@RobinL RobinL closed this Oct 16, 2023
@RobinL RobinL deleted the janitor_class branch August 12, 2024 10:06
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