Skip to content

Fix analytics event documentation formatting#11727

Merged
zachmargolis merged 7 commits intomainfrom
margolis-fix-analytics-event-documentation
Jan 11, 2025
Merged

Fix analytics event documentation formatting#11727
zachmargolis merged 7 commits intomainfrom
margolis-fix-analytics-event-documentation

Conversation

@zachmargolis
Copy link
Copy Markdown
Contributor

Before

The description included some notes from the new_device attribute

> make analytics_events
> jq -r '.events[] | select( .event_name | contains("Email and Password") ) | .description' < public/api/_analytics-events.json
the attempt was unsuccessful, since it cannot be known whether it's a new device.
Tracks authentication attempts at the email/password screen

After

The description is only the description

> make analytics_events
> jq -r '.events[] | select( .event_name | contains("Email and Password") ) | .description' < public/api/_analytics-events.json
Tracks authentication attempts at the email/password screen

@zachmargolis zachmargolis requested a review from a team January 8, 2025 23:53
- "Email and Password Authentication" description was wrong
- Similar to #11640

changelog: Internal, Documentation, Fix documentation formatting
@zachmargolis zachmargolis force-pushed the margolis-fix-analytics-event-documentation branch from c42174a to 7561d6a Compare January 9, 2025 00:08
Copy link
Copy Markdown
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 👍

Another idea for enforcing this: Maybe we can require that the method description starts uppercase? That might be enough of a hint to people in scenarios like this.

@zachmargolis
Copy link
Copy Markdown
Contributor Author

Another idea for enforcing this: Maybe we can require that the method description starts uppercase? That might be enough of a hint to people in scenarios like this.

Oh that's a great idea! Let me give that a shot

@zachmargolis
Copy link
Copy Markdown
Contributor Author

Another idea for enforcing this: Maybe we can require that the method description starts uppercase? That might be enough of a hint to people in scenarios like this.

Oh that's a great idea! Let me give that a shot

Ok so after doing that, a few updates!

  • Basic lowercase checking in ac7a2ec
  • Turns out # rubocop:disable FooBar lines were getting included in descriptions and flagging this error, so in 3a6c537 I updated to strip those out, which is a win for the resulting documentation too
  • Fixed errors in 46fa55c, which included capitalizing some desciptions

The errors now look like

app/services/analytics_events.rb:7109 sms_opt_in_visit method description starts with lowercase, check indentation

which hopefully is clear to future us what we are trying to fix

Copy link
Copy Markdown
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.

Big wins all around!

Comment on lines +132 to +135
description = method_description(method_object)
if description.present? && !method_object.docstring.match?(/\A[A-Z]/)
errors << "#{error_prefix} method description starts with lowercase, check indentation"
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we include the parsed description in the error message so it'll be even more obvious if the description includes param text? For context, I was previously unaware of the indentation requirement, so "check indentation" probably wouldn't have been enough for me to realize if a description includes parameter text.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about 24e3453, which would look like this:

app/services/analytics_events.rb:249 add_email_confirmation method description starts with lowercase, check indentation:
  selection.
  A user has clicked the confirmation link in an email"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wfm 👍

@zachmargolis zachmargolis merged commit c32366f into main Jan 11, 2025
@zachmargolis zachmargolis deleted the margolis-fix-analytics-event-documentation branch January 11, 2025 00:02
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.

3 participants