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

Always convert CSS classes to system arguments in linters #2445

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

camertron
Copy link
Contributor

What are you trying to accomplish?

We received a Slack message last week about trying to get the Primer/SystemArgumentInsteadOfClass linter working in another repo. I was able to get it running eventually, but couldn't get it to find any violations, even ones I'd added manually. I discovered the linter calls ::Primer::Classify::Utilities.classes_to_hash, which bails out early if primer/view_components is configured to skip class name validation. I brought the issue to the team and learned the code returns early to avoid performance issues in production. The thing is, we never call the classes_to_hash method in production lol so I got rid of it. This should make the linter work without additional configuration.

Integration

No changes necessary in production.

List the issues that this change affects.

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

Copy link

changeset-bot bot commented Dec 11, 2023

🦋 Changeset detected

Latest commit: 43927be

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@camertron camertron force-pushed the always_validate_class_names_for_linters branch from b6d26a4 to 6ef4ae1 Compare December 11, 2023 22:47
@camertron camertron marked this pull request as ready for review December 11, 2023 22:47
@camertron camertron requested review from a team and keithamus December 11, 2023 22:47
@camertron camertron merged commit da60c73 into main Dec 13, 2023
29 checks passed
@camertron camertron deleted the always_validate_class_names_for_linters branch December 13, 2023 21:17
@primer primer bot mentioned this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants