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

New function check_cardinality() #15

Merged
merged 14 commits into from Nov 27, 2019
Merged

New function check_cardinality() #15

merged 14 commits into from Nov 27, 2019

Conversation

TSchiefer
Copy link
Member

  • returns the nature of the relationship between parent_table$pk_col and child_table$fk_col

@TSchiefer TSchiefer requested a review from krlmlr July 23, 2019 16:21
Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Good idea.

  • Do we need an is_cardinality...() for each check_cardinality(), would that remove the need for a verbose argument?

  • What would be a return value that works for both humans and machines? A number with a string-valued attribute perhaps?

R/check-cardinalities.R Outdated Show resolved Hide resolved
ct <- enquo(child_table)
fkc <- enexpr(fk_column)

check_key(!!pt, !!pkc)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this abort the function if not a primary key?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this is intended (parent table has to have a primary key; this needs to be adapted as soon as compound keys are supported)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Before moving on, let's investigate (in a new branch) if is_cardinality_...() functions will make the implementation easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, but would you like to generally avoid the verbose argument, also in check_if_subset() and check_set_equality()? In this case we might need a more basic version of these two functions, which would be called by their counterparts, right? We could then use those basic functions for is_cardinality...().

Is that how you intended this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the verbose argument won't be necessary then.

I'm not sure why we settled for exceptions, but with the new check_cardinality() function it seems like we really should go for functions that check without throwing exceptions. I'd like to play with that code myself first.

Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored and removed the verbose-argument

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@TSchiefer TSchiefer merged commit fba3db1 into master Nov 27, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants