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

Projects
None yet
4 participants
@glentennis
Copy link
Contributor

commented Feb 22, 2019

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

This comment has been minimized.

Copy link

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

@glentennis glentennis force-pushed the glentennis:new-var-types branch from 0d4ec8f to 8da483f Feb 22, 2019

@codecov

This comment has been minimized.

Copy link

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],

This comment has been minimized.

Copy link
@gsheni

gsheni Feb 22, 2019

Contributor

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"""

This comment has been minimized.

Copy link
@gsheni

gsheni Feb 22, 2019

Contributor

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"""

This comment has been minimized.

Copy link
@gsheni

gsheni Feb 22, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

@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

This comment has been minimized.

Copy link
Contributor

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 glentennis force-pushed the glentennis:new-var-types branch from 8d1a410 to 01cb2ed Feb 25, 2019

@glentennis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

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

@gsheni

gsheni approved these changes Feb 25, 2019

@glentennis glentennis force-pushed the glentennis:new-var-types branch 3 times, most recently from c9e3d64 to 058a3e1 Feb 25, 2019

@kmax12

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@glentennis fyi _dtype_repr changed to type_string in #361

@glentennis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

thanks! totally missed that

@glentennis glentennis force-pushed the glentennis:new-var-types branch from 6ff2c8d to 40dd429 Feb 25, 2019

@glentennis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@gsheni checks passed, can you merge this?

@gsheni gsheni merged commit 5d4bd73 into Featuretools:master Feb 26, 2019

3 checks passed

codecov/patch 100% of diff hit (target 96.45%)
Details
codecov/project 96.46% (+<.01%) compared to 4bf6709
Details
license/cla Contributor License Agreement is signed.
Details

@glentennis glentennis deleted the glentennis:new-var-types branch Feb 26, 2019

@rwedge rwedge referenced this pull request Mar 29, 2019

Merged

v0.7.0 #477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.