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.

Sign up for GitHub

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

Jump to bottom

fix 151 and 374 - cascades to part tables must be from parent tables. #957

Merged
merged 27 commits into from
Jan 20, 2022
Merged

fix 151 and 374 - cascades to part tables must be from parent tables. #957

merged 27 commits into from
Jan 20, 2022

Conversation

dimitri-yatsenko
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko commented Sep 9, 2021 •
edited
Loading

fix #151 (Method 1)
fix #374

@dimitri-yatsenko dimitri-yatsenko changed the title fix issue 151 - cascades to part tables must be from parent tables. fix 151 and 374 - cascades to part tables must be from parent tables. Sep 10, 2021
@dimitri-yatsenko dimitri-yatsenko marked this pull request as ready for review September 10, 2021 17:57
guzman-raphael
guzman-raphael requested changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@guzman-raphael guzman-raphael left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @dimitri-yatsenko. Logic looks good. 🤝
Requesting a fix of just some minor documentation errors (for Sphinx rendering) and some 'future-proof' suggestions for the tests (use built in assert). Also, please link the issues that are fixed here so they are auto-closed on merge.

CHANGELOG.md Outdated Show resolved Hide resolved
docs-parts/intro/Releases_lang1.rst Outdated Show resolved Hide resolved
tests/schema_simple.py
@@ -4,6 +4,10 @@
import random
import datajoint as dj
import itertools
import hashlib
import uuid
import faker
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should set the seed here to make this test deterministic.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is already deterministic. The code generates deterministic UUIDs rather than random.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dimitri-yatsenko That is not what I meant. UUID's are generated from url which is actually generated by faker's profile method. This is the portion I meant by not being deterministic. Each time we call fake.profile(), it will yield a random result unless we seed it properly.

tests/test_cascading_delete.py Outdated Show resolved Hide resolved
tests/test_cascading_delete.py Outdated Show resolved Hide resolved
datajoint/table.py Outdated Show resolved Hide resolved
datajoint/table.py Outdated Show resolved Hide resolved
datajoint/table.py Outdated Show resolved Hide resolved
datajoint/table.py Outdated Show resolved Hide resolved
datajoint/utils.py Outdated Show resolved Hide resolved
dimitri-yatsenko and others added 12 commits September 14, 2021 11:34
@dimitri-yatsenko
Merge branch 'master' of https://github.com/datajoint/datajoint-python …
ce3e4ab 
…into issue151
@dimitri-yatsenko @guzman-raphael
add type hinting in utils.get_master
d1dd910 
Co-authored-by: Raphael Guzman <[email protected]>
@dimitri-yatsenko
Merge branch 'issue151' of https://github.com/dimitri-yatsenko/datajo…
9e13c21 
…int-python into issue151
@dimitri-yatsenko
minor cleanup in tests
bd16e0f
@dimitri-yatsenko
switch from nosetest to pytest asserts in test_stepwise_delete
17d3bb4
@dimitri-yatsenko
Merge branch 'master' of https://github.com/datajoint/datajoint-python …
24ba679 
…into issue151
@dimitri-yatsenko
add random seed to faker in tests.schema_simple to make tests determi…
f505335 
…nistic
@dimitri-yatsenko
Merge branch 'master' of https://github.com/datajoint/datajoint-python …
07ab459 
…into issue151
@dimitri-yatsenko
set RNG seed for faker in tests
58a294b
@dimitri-yatsenko @guzman-raphael
docstring improvement in datajoint/table.py
67dda6c 
Co-authored-by: Raphael Guzman <[email protected]>
@dimitri-yatsenko @guzman-raphael
fix grammar in docstring
a56d634 
Co-authored-by: Raphael Guzman <[email protected]>
@dimitri-yatsenko @guzman-raphael
Apply suggestions from code review
54d4e46 
docstring improvements in datajoint.table.

Co-authored-by: Raphael Guzman <[email protected]>
@dimitri-yatsenko dimitri-yatsenko marked this pull request as draft September 21, 2021 19:03
@dimitri-yatsenko dimitri-yatsenko marked this pull request as ready for review September 23, 2021 18:46
@dimitri-yatsenko dimitri-yatsenko mentioned this pull request Sep 24, 2021
Closed
@A-Baji
Copy link
Collaborator

A-Baji commented Jan 20, 2022

@dimitri-yatsenko Please update this branch so I can merge.

guzman-raphael
guzman-raphael approved these changes Jan 20, 2022
View reviewed changes
Copy link
Collaborator

@guzman-raphael guzman-raphael left a comment

Choose a reason for hiding this comment

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

@dimitri-yatsenko Great work! 🚀

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
### 0.13.3 -- TBD
* Bugfix - Dependencies not properly loaded on populate. (#902) PR #919
* Bugfix - Replace use of numpy aliases of built-in types with built-in type. (#938) PR #939
* Bugfix - Deletes and drops must include the master of each part. (#151 and #374) PR #957
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Bugfix - Deletes and drops must include the master of each part. (#151 and #374) PR #957
* Bugfix - Deletes and drops must include the master of each part. (#151, #374) PR #957

@guzman-raphael guzman-raphael merged commit 4afa476 into datajoint:master Jan 20, 2022
@dpeg22 dpeg22 mentioned this pull request Sep 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Baji A-Baji A-Baji approved these changes

@guzman-raphael guzman-raphael guzman-raphael approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drop all parts when master is dropped in a master-part relationship. Cascading delete for Part
3 participants
@dimitri-yatsenko @A-Baji @guzman-raphael