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

[TECH DEBT] Update the signature of _is_migrated function to only get table names as input #2397

Closed
aminmovahed-db opened this issue Aug 9, 2024 · 1 comment · Fixed by #3200
Labels
tech debt chores and design flaws

Comments

@aminmovahed-db
Copy link
Contributor

On one hand, I would like this method to have the following signature: _is_migrated(self, table: Table) -> bool. To me that makes more sense, it is "better" OO programming and it is probably more future proof (what if we want to include the catalog at some point).

On the other hand, I do not think you have to change it because: i) the index.is_migrated has the same signature, so it is better to be consitent now, log this as a (small) tech debt issue and do refactoring in a separate PR. (We prefer to separate refactoring from feature implementation.) and ii) it is a hidden method, therefore we can refactor the signature without worrying about changing the public API

Originally posted by @JCZuurmond in #2205 (comment)

@JCZuurmond JCZuurmond added the tech debt chores and design flaws label Aug 12, 2024
@JCZuurmond JCZuurmond changed the title Update the signature of _is_migrated function to only get table names as input [TECH DEBT] Update the signature of _is_migrated function to only get table names as input Aug 12, 2024
@JCZuurmond
Copy link
Contributor

@aminmovahed-db : could you link the code location and update the first comment to follow our template issue structure?

@nfx nfx moved this from Triage to Quarter Backlog in UCX (roadmap) Oct 9, 2024
@nfx nfx closed this as completed in #3200 Nov 11, 2024
@nfx nfx closed this as completed in 403e4fd Nov 11, 2024
@github-project-automation github-project-automation bot moved this from Quarter Backlog to Archive in UCX (roadmap) Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt chores and design flaws
Projects
Archived in project
2 participants