Skip to content

Add click url#19

Merged
dmosorast merged 3 commits intosinger-io:masterfrom
amyfain:add-click-url
Nov 27, 2018
Merged

Add click url#19
dmosorast merged 3 commits intosinger-io:masterfrom
amyfain:add-click-url

Conversation

@amyfain
Copy link
Contributor

@amyfain amyfain commented Nov 13, 2018

I'd like to add click URL to the Events stream and also reduce the bookmark rewind window.

@cmerrick
Copy link
Contributor

Hi @amyfain, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@cmerrick
Copy link
Contributor

You did it @amyfain!

Thank you for signing the Singer Contribution License Agreement.

'URL': {
'type': ['null','string'],
'description': 'URL that was clicked.'
},

Choose a reason for hiding this comment

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

Assuming you run this tap yourself, when you add this URL to the schema and run the tap, do you see this new data being emitted?

If not, I suspect that we need to add 'URL' to the KEY_PROPERTIES list on line 39.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I tested this by adding URL to the schema, and it runs without URL being a KEY_PROPERTY.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luandy64 do I need to add URL to KEY_PROPERTY?

@dmosorast
Copy link
Contributor

Hi @amyfain, I'm picking this up for Andy. Looking at the code, it looks good the way this is written, no need to add this field to the KEY_PROPERTIES, since it only appears on the Click event type. The key properties are fields that determine a unique record within the stream.

The event stream leaves a bit to be desired the way it is written, since it requests data from many different event types, it seems like the schema should be a superset of all the possible options, differentiated by EventType. Alternatively, it could be broken out into multiple streams to handle click, bounce, etc. events separately.

I think the way to go as written would be to include all of the possible event fields in the schema, and since this is a step in that direction, I'm good to merge it!

@dmosorast dmosorast dismissed luandy64’s stale review November 27, 2018 21:21

No need for the change requested.

@dmosorast dmosorast merged commit b04f6b1 into singer-io:master Nov 27, 2018
@amyfain
Copy link
Contributor Author

amyfain commented Nov 28, 2018

Awesome!! Thanks so much @dmosorast !

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