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

[CT-2559] [Feature] Remove unused check_new method #7586

Closed
3 tasks done
dbeatty10 opened this issue May 10, 2023 · 3 comments · Fixed by #7756
Closed
3 tasks done

[CT-2559] [Feature] Remove unused check_new method #7586

dbeatty10 opened this issue May 10, 2023 · 3 comments · Fixed by #7756
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors! tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality

Comments

@dbeatty10
Copy link
Contributor

Is this your first time submitting a feature request?

  • I have read the expectations for open source contributors
  • I have searched the existing issues, and I could not find an existing issue for this feature
  • I am requesting a straightforward extension of existing dbt functionality, rather than a Big Idea better suited to a discussion

Describe the feature

It looks like check_new is unused after being removed in an earlier PR. If so, then could we remove check_new altogether?

def check_new(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool:
return old is None

I didn't do a git grep "check_new" in dbt-core or any adapters, but didn't see any usage here:
https://github.com/search?q=repo%3Adbt-labs%2Fdbt-core+check_new&type=code

Describe alternatives you've considered

We can leave dead code like this without creating problems, but always nice to do some spring cleaning.

Who will this benefit?

Anyone reading or developing this portion of the code base.

Are you interested in contributing this feature?

No response

Anything else?

No response

@dbeatty10 dbeatty10 added enhancement New feature or request tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality labels May 10, 2023
@github-actions github-actions bot changed the title [Feature] Remove unused check_new method [CT-2559] [Feature] Remove unused check_new method May 10, 2023
@shivu2002a
Copy link

Hi @dbeatty10 ,
Can I take up this issue please? This method is to be removed, right?

@dbeatty10
Copy link
Contributor Author

Yep you got it @shivu2002a 👍 Thanks for jumping in ❤️

During code review of your pull request, you and the reviewer can confirm/deny if it is safe to fully remove it. I think the answer is "yes", but I didn't try it out myself.

One other thing to note when you open a PR -- here are things I see forgotten most often for first-time contributors:

@shivu2002a
Copy link

@dbeatty10 Sure thing. I'll try submitting a pr. Thanks 😊

@jtcohen6 jtcohen6 added the good_first_issue Straightforward + self-contained changes, good for new contributors! label May 30, 2023
kevinneville added a commit to kevinneville/dbt-core that referenced this issue Jun 1, 2023
kevinneville added a commit to kevinneville/dbt-core that referenced this issue Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors! tech_debt Behind-the-scenes changes, with little direct impact on end-user functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants