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

Add PhoneNumber, DateOfBirth, URL variable types #447

Merged
merged 4 commits into from
Feb 26, 2019

Conversation

glentennis
Copy link
Contributor

closes #443

@gsheni wanna take a look? I wasn't exactly sure if I was supposed to make changes to featuretools/tests/entityset_tests/test_entity.py, but I took a stab at it anyway.

Also made a couple changes to unrelated files via make lint-fix

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2019

CLA assistant check
All committers have signed the CLA.

@glentennis glentennis changed the title initial commit Add PhoneNumber, DateOfBirth, URL variable types Feb 22, 2019
@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #447 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
+ Coverage   96.45%   96.46%   +<.01%     
==========================================
  Files          98       98              
  Lines        8611     8620       +9     
==========================================
+ Hits         8306     8315       +9     
  Misses        305      305
Impacted Files Coverage Δ
featuretools/tests/testing_utils/mock_ds.py 87.76% <ø> (ø) ⬆️
featuretools/tests/entityset_tests/test_entity.py 100% <100%> (ø) ⬆️
featuretools/variable_types/variable.py 98.13% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bf6709...40dd429. Read the comment docs.

'engagement_level': [1, 3, 2],
'email': ['john.smith@example.com', '', np.nan],
'url': ['google.com', 'https://www.featuretools.com/', np.nan],
Copy link
Contributor

Choose a reason for hiding this comment

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

URL is property better to put in the products dataframe.

@@ -257,6 +257,24 @@ class EmailAddress(Variable):
_default_pandas_dtype = str


class URL(Variable):
"""Represents a valid web url"""
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put more information in this docstring. Saying that having a url with/without http and www is okay.



class PhoneNumber(Variable):
"""Represents a valid phone number"""
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put more information in this docstring. Saying that having a phone number with/without area and country code is okay, as well as a phone number with parentheses.

@glentennis
Copy link
Contributor Author

@gsheni addressed your comments! if this looks good, I can squash and re-push, which I think will take care of the CLA issue

@gsheni
Copy link
Contributor

gsheni commented Feb 25, 2019

@glentennis you need to agree to the Contributor License Agreement, please follow these steps
screen shot 2019-02-25 at 10 33 12 am


Then click here:
screen shot 2019-02-25 at 10 33 03 am

Or try click this link
https://cla-assistant.io/Featuretools/featuretools?pullRequest=447

@glentennis
Copy link
Contributor Author

Hey @gsheni, looks like we're good to go on this!

@glentennis glentennis force-pushed the new-var-types branch 3 times, most recently from c9e3d64 to 058a3e1 Compare February 25, 2019 23:14
@kmax12
Copy link
Contributor

kmax12 commented Feb 25, 2019

@glentennis fyi _dtype_repr changed to type_string in #361

@glentennis
Copy link
Contributor Author

thanks! totally missed that

@glentennis
Copy link
Contributor Author

@gsheni checks passed, can you merge this?

@gsheni gsheni merged commit 5d4bd73 into alteryx:master Feb 26, 2019
@glentennis glentennis deleted the new-var-types branch February 26, 2019 03:38
@rwedge rwedge mentioned this pull request Mar 29, 2019
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.

Add PhoneNumber, DateOfBirth, URL variable types
4 participants