Skip to content

Added extraction of postal address#5

Open
Sub-Zero-1 wants to merge 3 commits intoJettF:developfrom
Sub-Zero-1:master
Open

Added extraction of postal address#5
Sub-Zero-1 wants to merge 3 commits intoJettF:developfrom
Sub-Zero-1:master

Conversation

@Sub-Zero-1
Copy link

No description provided.

@JettF JettF self-requested a review June 17, 2018 17:46
@JettF
Copy link
Owner

JettF commented Jun 17, 2018

My apologies for taking so long to get to this. I looked over what you've done and I like it. I am thinking we might want a way to configure whether the postal address is included or not because it seems to add a significant amount of data to each contact and I don't want to slow down the user experience.

In the issue you opened you mentioned that the PR needed some modifications. What sort of modifications were you thinking of?

@TomWFox
Copy link

TomWFox commented Feb 11, 2019

Is there any chance of this getting merged?

@AnthonyMDev
Copy link
Collaborator

@Tom-Fox1 could you answer @JettF 's questions from above? If you could confirm that this doesn't have any more modifications that need to be made, and just fix the merge conflict, I'll ensure it gets merged in.

@AnthonyMDev
Copy link
Collaborator

@JettF as far as determining which fields are included, that should probably be the case for all fields. We should create an enum for the fields and pass it in the initializer, probably with a default value.

@TomWFox
Copy link

TomWFox commented Feb 12, 2019

@AnthonyMDev I didn't author this PR so don't know about the potential need for modification mentioned by @Sub-Zero-1.

What @JettF said about adding address a a config option to keep a fast UX may still need to be looked at.

I just chimed in as this is something that is make or break for my use case.

@AnthonyMDev
Copy link
Collaborator

AnthonyMDev commented Feb 12, 2019 via email

@TomWFox
Copy link

TomWFox commented Feb 12, 2019

No worries, thanks for responding so quickly!

@AnthonyMDev AnthonyMDev self-assigned this Feb 20, 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.

4 participants