Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Revert "Fix Violations of and Reenable Style/RescueModifier"" #58030

Merged
merged 3 commits into from Apr 17, 2024

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Apr 16, 2024

Reverts #58029, reapplying #57847

The original PR failed linting when it hit staging, because neither my manual local testing nor automated Drone testing linted haml, and we had a few violations of the new rule hidden in some of our haml templates.

I manually fixed those violations, and verified with bundle exec haml-lint that there are no more.


%h1= hoc_s(:events_all_old_title).gsub(/\@year/, hoc_year.to_s)

%ul
%li{class: "hoc-event-country"}
%span{style: "font-weight: bold;"}= "United States (#{us_events.count rescue '0'} #{hoc_s(:events)})"
%span{style: "font-weight: bold;"}= "United States (#{us_events_count} #{hoc_s(:events)})"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is actually a slight refactoring of this very old (and likely entirely unused) page, to better align with how newer pages work. Because us_events is a group of collections, simply calling .count on it will return the number of collections in the group, not the number of events in all the collections.

@Hamms Hamms marked this pull request as ready for review April 17, 2024 00:11
@Hamms Hamms requested review from a team as code owners April 17, 2024 00:11
@Hamms Hamms requested a review from a team April 17, 2024 00:11
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

updates look good! yay for haml linting but boo for not running it until staging.

do you have a sense of when the appropriate time to run haml-lint would be, of the places you mentioned that it's not currently being run?

@Hamms
Copy link
Contributor Author

Hamms commented Apr 17, 2024

Oh, to clarify; Drone will lint HAML files, but only HAML files that have been modified in the PR itself.

We could consider linting every file in the codebase for every drone run, but at a cost of slowing down all our drone runs. See #55312 for some previous discussion on the topic

@Hamms Hamms merged commit e3fb7cb into staging Apr 17, 2024
2 checks passed
@Hamms Hamms deleted the revert-58029-revert-57847-fix-Style/RescueModifier branch April 17, 2024 17:15
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.

None yet

2 participants