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

Add selector method capabilities to selectors #4827

Conversation

danieldiamond
Copy link
Contributor

@danieldiamond danieldiamond commented Mar 5, 2022

resolves #4821

Description

Add selector method in order to be able to define a selector with another selector definition.

To Do

  • Prevent circular dependencies
  • unit test to ensure circular dependency check works
  • unit test to confirm selector method has expected behaviour

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@danieldiamond danieldiamond requested review from a team as code owners March 5, 2022 14:58
@cla-bot cla-bot bot added the cla:yes label Mar 5, 2022
@danieldiamond
Copy link
Contributor Author

Initial attempt, seems to be working but not such a fan of injecting the selector_dict through the class methods, particularly as we're updating it (hence the deepcopy). Ideally, this would be an instantiated class to lean on self.selector_dict or similar.

@danieldiamond danieldiamond requested a review from a team March 6, 2022 04:08
@danieldiamond
Copy link
Contributor Author

@jtcohen6 any capacity this month for the team to review?

Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I see that you have a checkbox unchecked for circular dependencies though. Do you have an idea of how to handle that? It might be tricky to handle robustly, but it would be good to create a test case with circular dependencies and see what error occurs. Catching the error at a higher level would probably be fine (if possible). A python error for recursiveness would be not optimal...

@gshank
Copy link
Contributor

gshank commented Apr 7, 2022

Oh, also we now use 'changie' for the changelog, so the CHANGELOG.md should not be changed from main. See: https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md#adding-changelog-entry

@danieldiamond
Copy link
Contributor Author

I see that you have a checkbox unchecked for circular dependencies though. Do you have an idea of how to handle that?

I was hoping to get some guidance on this. Is this something you can help with @gshank ?
Something as simple as checking if the selector key already exists then error out as "already defined at root" or similar

@gshank
Copy link
Contributor

gshank commented Apr 8, 2022

That part will probably be a bit tricky. I think we'd have to keep a list of already encountered selectors in this branch of the tree and check if this selector was encountered higher up. I haven't touched this code in a while, so I'd have to pull it apart a bit further to figure out where that would have to go. For recursive functions, I generally just pass along a list of names that I check...

@danieldiamond
Copy link
Contributor Author

@gshank any chance I can get some help to get this over the line? Please and thank you in advance!

@gshank
Copy link
Contributor

gshank commented Apr 19, 2022

I won't have time today, but I'll try to find some time to take a look this week.

@gshank
Copy link
Contributor

gshank commented Apr 26, 2022

I did some testing and it looks like we don't end up with recursion, we just get "Existing selector definition for not found." Which is pretty innocuous.

@gshank gshank self-requested a review April 26, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-319] [Feature] Selection method based on a selection definition
3 participants