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

Multi table insert #126

Merged
merged 27 commits into from
Aug 31, 2022
Merged

Multi table insert #126

merged 27 commits into from
Aug 31, 2022

Conversation

A-Baji
Copy link
Collaborator

@A-Baji A-Baji commented Aug 12, 2022

No description provided.

@A-Baji A-Baji marked this pull request as ready for review August 15, 2022 20:07
Copy link
Contributor

@jverswijver jverswijver left a comment

Choose a reason for hiding this comment

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

Looks great! If you have extra time you should add a test for the form component in the testing suite.

Copy link
Contributor

@jverswijver jverswijver left a comment

Choose a reason for hiding this comment

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

Sorry did not see that the tests did not pass. I spent some time looking at it and I think I know what the problem is, I will highlight it in this review.

pharus/dynamic_api_gen.py Outdated Show resolved Hide resolved
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.

@A-Baji Nice job in figuring a lot of this out. 💪

Based on this, we can tweak our structure a bit to properly accommodate.

pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Show resolved Hide resolved
pharus/dynamic_api_gen.py Outdated Show resolved Hide resolved
tests/init/test_dynamic_api_spec.yaml Show resolved Hide resolved
@A-Baji A-Baji requested a review from guzman-raphael August 16, 2022 22:05
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.

@A-Baji Looking good. 😎

We should add a GET route for the fields info since the frontend will need this. This is necessary so we can map between the display and what the database needs. I've updated the issue with specifications. Could you look into that next?

pharus/component_interface.py Outdated Show resolved Hide resolved
@A-Baji A-Baji requested a review from jverswijver August 24, 2022 19:08
@A-Baji A-Baji requested a review from guzman-raphael August 24, 2022 19:37
@guzman-raphael guzman-raphael linked an issue Aug 26, 2022 that may be closed by this pull request
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.

@A-Baji Great job! 👍 Can see a well thought-out approach here.

Have some additional points for consideration in your algorithm approach. I actually have an example to share that can help you improve on all of these. 🤓

Oh, and what if I could tell you we could get the following as a bonus too?

  • Allows multiple inserts to be submitted
  • Configuration can omit individual maps if no change
  • Configuration can omit map from table maps if no change
  • slightly less code

Find me and we can knock this out in a tagup together.

pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
pharus/component_interface.py Outdated Show resolved Hide resolved
guzman-raphael and others added 4 commits August 30, 2022 16:31

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Allow multiple submissions, maps are now individually optional.
@jverswijver jverswijver merged commit c17ee2c into datajoint:master Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transaction LC Multi-Insert
3 participants