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

get table metadata and manage dataset privacy #691

Merged
merged 29 commits into from
May 28, 2019
Merged

get table metadata and manage dataset privacy #691

merged 29 commits into from
May 28, 2019

Conversation

simon-contreras-deel
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel commented May 22, 2019 •
edited
Loading

Solves #673

Main implementation points:

  • Creation of DatasetInfo class wrapping the dataset metadata properties and calls to CARTO
  • Dataset has 2 new methods, to read/write privacy and name:
dataset.get_dataset_info().privacy  # PRIVATE, PUBLIC, LINK
dataset.update_dataset_info(privacy=Dataset.PUBLIC)
  • Mocks creation for testing avoiding end to end tests and doing them faster. Taking advantage of them, I have refactorized test_dataset_sync.py too (28 seconds to 1).

houndci-bot
houndci-bot reviewed May 23, 2019
View reviewed changes
cartoframes/dataset.py Outdated Show resolved Hide resolved
cartoframes/dataset.py Outdated Show resolved Hide resolved
@simon-contreras-deel simon-contreras-deel changed the title get table metadata and manage dataset privacy [WIP] get table metadata and manage dataset privacy May 24, 2019
houndci-bot
houndci-bot reviewed May 24, 2019
View reviewed changes
test/test_dataset.py Show resolved Hide resolved
test/test_dataset.py Outdated Show resolved Hide resolved
test/mocks/dataset_mock.py Show resolved Hide resolved
test/mocks/dataset_mock.py Outdated Show resolved Hide resolved
test/mocks/dataset_mock.py Show resolved Hide resolved
test/mocks/context_mock.py Show resolved Hide resolved
test/mocks/context_mock.py Show resolved Hide resolved
houndci-bot
houndci-bot reviewed May 24, 2019
View reviewed changes
cartoframes/dataset_info.py Outdated Show resolved Hide resolved
cartoframes/dataset_info.py Outdated Show resolved Hide resolved
@simon-contreras-deel simon-contreras-deel changed the title [WIP] get table metadata and manage dataset privacy get table metadata and manage dataset privacy May 24, 2019
alrocar
alrocar reviewed May 27, 2019
View reviewed changes
Copy link
Contributor

@alrocar alrocar 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.

I left a bunch of comments, some are proposals and some others are there to know your opinion :)

I like having the DatasetInfo class as a wrapper with the minimal stuff we want to let the users actually see/update.

cartoframes/dataset.py Show resolved Hide resolved
cartoframes/dataset.py Outdated Show resolved Hide resolved
cartoframes/dataset.py Outdated Show resolved Hide resolved
cartoframes/dataset.py Show resolved Hide resolved
cartoframes/dataset_info.py Outdated Show resolved Hide resolved
cartoframes/dataset_info.py Outdated Show resolved Hide resolved
cartoframes/dataset_info.py Outdated Show resolved Hide resolved
cartoframes/dataset_info.py Outdated Show resolved Hide resolved
cartoframes/dataset.py Outdated Show resolved Hide resolved
ethervoid
ethervoid reviewed May 27, 2019
View reviewed changes
test/mocks/context_mock.py
# -*- coding: utf-8 -*-


class CredsMock():
Copy link
Contributor

Choose a reason for hiding this comment

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

One question. Did you look for a mock library FactoryGirl style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nop

@simon-contreras-deel
Copy link
Contributor Author

simon-contreras-deel commented May 27, 2019 •
edited
Loading

I have handled the last comments on my own. Can you take another CR round? 🙏

alrocar
alrocar reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@alrocar alrocar left a comment

Choose a reason for hiding this comment

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

Great job addressing most of our comments 💪

I left just one more 👼

cartoframes/datasets/dataset_info.py Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed May 28, 2019
View reviewed changes
cartoframes/datasets/dataset_info.py Outdated Show resolved Hide resolved
cartoframes/datasets/dataset_info.py Show resolved Hide resolved
cartoframes/datasets/__init__.py Show resolved Hide resolved
alrocar
alrocar approved these changes May 28, 2019
View reviewed changes
Copy link
Contributor

@alrocar alrocar left a comment

Choose a reason for hiding this comment

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

🚀

@simon-contreras-deel simon-contreras-deel merged commit b0c1d26 into feature-sharing-viz May 28, 2019
@simon-contreras-deel simon-contreras-deel deleted the dataset-metadata branch May 28, 2019 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@ethervoid ethervoid ethervoid left review comments

@alrocar alrocar alrocar approved these changes

@andy-esch andy-esch Awaiting requested review from andy-esch

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

Successfully merging this pull request may close these issues.
4 participants
@simon-contreras-deel @alrocar @ethervoid @houndci-bot