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

Clean old API #871

Merged
merged 11 commits into from
Jul 25, 2019
Merged

Clean old API #871

merged 11 commits into from
Jul 25, 2019

Conversation

oleurud
Copy link
Contributor

@oleurud oleurud commented Jul 24, 2019

Solves #807 & #624

  • Remove context.py and tests (also resolves Clean old API #807)
  • Remove layer.py and tests
  • Remove map.py and tests
  • Remove styling.py and tests
  • Remove assets/cartoframes.html
  • Move Columns.py to data?
  • Update examples (to use Dataset)

I have created a table file in data namespace to include the tables method from context

@oleurud oleurud mentioned this pull request Jul 24, 2019
2 tasks
@@ -122,6 +121,10 @@ def set_default_credentials(
_default_credentials.session = session


def _is_url(text):
return re.match(r'^https?://.*$', text)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the user enters http://username.carto.com? This will return False, but is that still a valid URL for API calls? If so, should we coerce it to https or give a warning?

Copy link
Member

Choose a reason for hiding this comment

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

http should work, because of the ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works, I checked it the first time I saw it

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. Should we coerce to https?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. base_url is used mainly by onpremises, right? In this case, I suppose we shouldn't require https. What do you see it?

@@ -110,7 +111,7 @@ class Examples(Context):
"""

def __init__(self):
super(Examples, self).__init__(
set_default_credentials(
Copy link
Member

Choose a reason for hiding this comment

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

Use self._credentials = Credentials(... instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@andy-esch andy-esch left a comment

Choose a reason for hiding this comment

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

🔥 🔥 🔥

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.

Looks good!

Just the change in the Example class about using scoped credentials instead of setting the default ones.

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.

🚀

@oleurud oleurud merged commit b861898 into develop Jul 25, 2019
@oleurud oleurud deleted the feature/kill-all-humans branch July 25, 2019 08:53
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.

Clean old API
4 participants