Skip to content

Added basic list_tables function for schema class#844

Merged
dimitri-yatsenko merged 7 commits intodatajoint:masterfrom
Synicix:master
Dec 11, 2020
Merged

Added basic list_tables function for schema class#844
dimitri-yatsenko merged 7 commits intodatajoint:masterfrom
Synicix:master

Conversation

@Synicix
Copy link
Contributor

@Synicix Synicix commented Dec 9, 2020

I just added what I needed for DJGUI for now, as such it returns the raw names to be parsed by the API on the other side.

I can add additional features as people request them like restricting by lookup tables are returning it in a different format.

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.

Thanks @Synicix for the PR. In addition to the provided inline feedback, I would suggest to add at least one test to validate the feature. Otherwise, our coverage will go down. It could simply be to create a table and then verify that it is returned when invoking the class method. Also, adding 'Fix #838' to the description of your PR will auto-close the issue once merged to the default branch.

Ugh, looks like there was a recent release (yesterday!) of minio python package that broke some S3 features. Such is the nature of development... Could you also track down the nature of the failure and see if it is easy to patch? You should be able to inspect the tests in GitHub Actions or locally to get a good idea of what is happening. Worst case scenario, we can version pin it to the last release. You can also file an issue regarding it.

@Synicix
Copy link
Contributor Author

Synicix commented Dec 9, 2020

Need to add tests, please wait for that commmit before reviewing

@Synicix Synicix marked this pull request as draft December 9, 2020 23:18
@Synicix
Copy link
Contributor Author

Synicix commented Dec 10, 2020

Will look into the minio errors in a different pull request

@Synicix Synicix marked this pull request as ready for review December 10, 2020 00:15
@Synicix
Copy link
Contributor Author

Synicix commented Dec 10, 2020

#847 In progress, probably will fix that first then have this update with that test fix to pass the requirements.

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.

See inline comment.

@Synicix
Copy link
Contributor Author

Synicix commented Dec 10, 2020

#838 Partially implmented for now

@dimitri-yatsenko
Copy link
Member

fix #838

@dimitri-yatsenko dimitri-yatsenko merged commit d97fe71 into datajoint:master Dec 11, 2020
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.

3 participants