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

Added list tables feature #1649

Merged
merged 3 commits into from
Jun 16, 2020
Merged

Added list tables feature #1649

merged 3 commits into from
Jun 16, 2020

Conversation

josemazo
Copy link
Contributor

Story ch70779

@josemazo josemazo requested a review from Jesus89 June 15, 2020 13:18
Copy link
Member

@Jesus89 Jesus89 left a comment

Choose a reason for hiding this comment

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

The current implementation does not cover the complete feature, because it does not allow to check the tables using the default_public API key, for example:

set_default_credentials('cartoframes')
list_tables()  # ForbiddenErrorException: Access denied

The original GitHub ticket links the previous implementation using the Python SDK (#1482, https://github.com/CartoDB/cartoframes/blob/v1.0b7/cartoframes/utils/table.py#L19). I think we should follow the same approach here and delegate the request in the DatasetManager:

datasets = DatasetManager(self.auth_client).filter(
    show_table_size_and_row_count='false',
    show_table='false',
    show_stats='false',
    show_likes='false',
    show_liked='false',
    show_permission='false',
    show_uses_builder_features='false',
    show_synchronization='false',
    load_totals='false')
tables = [str(dataset) for dataset in datasets]

Since this implementation does not require the schema I would remove it from thelist_tables function. I like using a DataFrame for the output, btw.

@josemazo
Copy link
Contributor Author

Ready for review again!

@Jesus89 Jesus89 merged commit ee2b443 into develop Jun 16, 2020
@Jesus89 Jesus89 deleted the josema/ch70779/list-carto-tables branch June 16, 2020 11:11
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.

2 participants