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

Uneeded error when delete on empty restriction in merge table #932

Closed
samuelbray32 opened this issue Apr 15, 2024 · 1 comment · Fixed by #940
Closed

Uneeded error when delete on empty restriction in merge table #932

samuelbray32 opened this issue Apr 15, 2024 · 1 comment · Fixed by #940
Assignees
Labels
bug Something isn't working merge To do with merge tables

Comments

@samuelbray32
Copy link
Collaborator

  • In the merge table delete function here it iterates of each part table with matching entries.
  • Merge.merge_get_part() return None if there are no matching parts. This is non-iterable and causes an error

Solution:
Either:

  • check return value of Merge.merge_get_part() before iterating here
  • change the return value of Merge.merge_get_part() for an empty case

@CBroz1, do you have any known reason to prefer either option?

@samuelbray32 samuelbray32 added bug Something isn't working merge To do with merge tables labels Apr 15, 2024
@CBroz1
Copy link
Member

CBroz1 commented Apr 15, 2024

Thanks for posting, Sam. It makes more sense to me for the delete to check the value. I don't think someone who used merge_get_part alone would expect an empty iterable in the null case, and I wouldn't want to change the behavior of one func to suit another. It's then a question of whether or not to flip return_empties - and I think it's more computation if we're running null deletes for each part. I might adjust delete to the following

class Merge(dj.Manual):
    ...
    def delete(self, force_permission=False, *args, **kwargs):
        """Alias for cautious_delete, overwrites datajoint.table.Table.delete"""
        if not (parts := self.merge_get_part(
            restriction=self.restriction,
            multi_source=True,
            return_empties=False,
        )):
            return
        for part in parts:        
            part.delete(force_permission=force_permission, *args, **kwargs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working merge To do with merge tables
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants