Skip to content

Sort analytics events#8519

Merged
matthinz merged 6 commits intomainfrom
matthinz/sort-analytics-events
Jun 1, 2023
Merged

Sort analytics events#8519
matthinz merged 6 commits intomainfrom
matthinz/sort-analytics-events

Conversation

@matthinz
Copy link
Contributor

analytics_events.rb is almost 4000 lines long, and the methods in it are not sorted alphabetically. This bugs me, so I sorted them and added a step to make lint that will error out if they're not kept sorted.

Does it make sense that I want to be able to scroll through this 4000 line file rather than just use the search function of my text editor? No, it does not. Is this a transparent attempt to juice my commit stats? Possibly. Is there a better way to do this, like with rubocop or something? Probably!

Testing

  1. Add a new method to analytics_events.rb, but purposefully DO NOT put it in the correct alphabetical position
  2. Run make lint_analytics_events_sorted and get yelled at
  3. Put your new method in the correct place
  4. Run make lint_analytics_events_sorted again and don't get yelled at

This is a noisy PR and likely to cause problems for people who have outstanding changes to analytics_events.rb. For this I apologize.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,6 +1,33 @@
# frozen_string_literal: true

# ______________________________________
# / Adding something new in here? Please \
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminds me of the time we added an ASCII dragon on an old project to highlight a load-bearing legacy file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that cowsay would increase the likelihood of people reading the message

matthinz added 2 commits June 1, 2023 08:46
it occurs to me that i should ensure the combo of make / sh / grep on the ci runner delivers the same behavior as it does locally
@matthinz matthinz merged commit 3e26e12 into main Jun 1, 2023
@matthinz matthinz deleted the matthinz/sort-analytics-events branch June 1, 2023 16:27
This was referenced Jun 6, 2023
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