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

feat(linter): support class sorting for tw.div #4699

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Yz4230
Copy link

@Yz4230 Yz4230 commented Dec 5, 2024

Summary

This PR adds support for class sorting in the tw.div syntax, commonly used with libraries such as twin.macro and react-twc.

- tw.div`px-2·foo·p-4·bar`;
+ tw.div`foo·bar·p-4·px-2`;

To enable sorting for tw.div or similar patterns, need to include a configuration entry like tw.* in options.functions.

Test Plan

  • Updated snapshots for useSortedClasses tests to reflect the added support for the tw.div syntax.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Dec 5, 2024
Copy link

codspeed-hq bot commented Dec 5, 2024

CodSpeed Performance Report

Merging #4699 will not alter performance

Comparing Yz4230:feat/class-sorting-tw-div (9896b1e) with main (5c51cb9)

Summary

✅ 97 untouched benchmarks

@github-actions github-actions bot added the A-Parser Area: parser label Dec 5, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Three things are missing in this PR:

  • A new changelog line that explains the new feature
  • Update the documentation, and explain the new feature. You should add something like **Since v2.0.0**
  • Update the documentation and add a new example with this new feature. The contribution guide explains how to do so

Also, one question: what was the reason for adding tw.* not tw.div?

Comment on lines 36 to 53
pub(crate) fn match_function(&self, name: &str) -> bool {
let matchers = self.functions.iter().flatten();
let parts = name.split('.');
for matcher in matchers {
let mut matcher = matcher.split('.');
let mut parts = parts.clone();

let mut zip = matcher.by_ref().zip(parts.by_ref());
if zip.all(|(m, p)| m == "*" || m == p)
&& matcher.next().is_none()
&& parts.next().is_none()
{
return true;
}
}

false
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub(crate) fn match_function(&self, name: &str) -> bool {
let matchers = self.functions.iter().flatten();
let parts = name.split('.');
for matcher in matchers {
let mut matcher = matcher.split('.');
let mut parts = parts.clone();
let mut zip = matcher.by_ref().zip(parts.by_ref());
if zip.all(|(m, p)| m == "*" || m == p)
&& matcher.next().is_none()
&& parts.next().is_none()
{
return true;
}
}
false
}
pub(crate) fn match_function(&self, name: &str) -> bool {
self.functions.iter().flatten().any(|matcher| {
let mut matcher_parts = matcher.split('.');
let mut name_parts = name.split('.').clone();
let all_parts_match = matcher_parts
.by_ref()
.zip(name_parts.by_ref())
.all(|(m, p)| m == "*" || m == p);
all_parts_match
&& matcher_parts.next().is_none()
&& name_parts.next().is_none()
})
}

uses .any() to replace the explicit loop and manual return true. the result is functionally equivalent, with the same time and space complexity.
I prepared some testcases and tested this on the playground, and the behavior matches the original implementation.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for suggesting this!🥹
Committed without using redundant .clone()! 2e70bca

-            let mut name_parts = name.split('.').clone();
+            let mut name_parts = name.split('.');

@github-actions github-actions bot added the A-Changelog Area: changelog label Dec 6, 2024
@Yz4230
Copy link
Author

Yz4230 commented Dec 6, 2024

@ematipico
Thank you very much for your review!
I added the changelog and updated the document.

Also, one question: what was the reason for adding tw.* not tw.div?

This is for supporting tags other than div such as tw.span or tw.a.

@Yz4230 Yz4230 requested a review from ematipico December 6, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter A-Parser Area: parser L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants