Skip to content

Document additional analytics events (LG-5924)#6149

Merged
zachmargolis merged 9 commits intomainfrom
margolis-analytics-events-6
Apr 5, 2022
Merged

Document additional analytics events (LG-5924)#6149
zachmargolis merged 9 commits intomainfrom
margolis-analytics-events-6

Conversation

@zachmargolis
Copy link
Contributor

This batch included DOC_AUTH which runs through the FSM and gets to be many many different events, I opted to skip that for now and focus on the more mechanical translations

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

stored_location:,
sp_request_url_present:,
remember_device:,
**extra
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to support extra?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but because we got burned by missing it one place, I made us require it for all methods in #6052. Maybe there's some additional documentation param we could no like a @noextra or something when we know but currently there's no exceptions to our lint on requiring that

Copy link
Contributor

@aduth aduth Apr 5, 2022

Choose a reason for hiding this comment

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

Gotcha, I thought that sounded familiar, but I swear I had seen another event where we weren't using **extra. Looking again, I don't see it, so must have been imagining things 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might have seen a zero-param event (those get a free pass) or seen one of the PRs prior to that one 🤷

zachmargolis and others added 2 commits April 5, 2022 09:01
@zachmargolis zachmargolis merged commit 325a575 into main Apr 5, 2022
@zachmargolis zachmargolis deleted the margolis-analytics-events-6 branch April 5, 2022 16:46
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.

2 participants