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(lint): display correspinding element #2930

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

fujiyamaorange
Copy link
Contributor

@fujiyamaorange fujiyamaorange commented May 21, 2024

Summary

Implements this comment.

Finally, we can get alternative element.

  ! The elements with the following roles can be changed to the following elements:
    <input type="checkbox">
  
  
    1  <>
  > 2      <div role="checkbox" ></div>
      │          ^^^^^^^^^^^^^^^

Test Plan

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

codspeed-hq bot commented May 21, 2024

CodSpeed Performance Report

Merging #2930 will not alter performance

Comparing fujiyamaorange:feat-get-corresponding-element (207c4f4) with main (71fe04e)

Summary

✅ 92 untouched benchmarks

@fujiyamaorange fujiyamaorange marked this pull request as ready for review May 21, 2024 09:15
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 @fujiyamaorange for making the rule better.

I left some suggestions.

There's one 'important change we need to do:

There's no need to collect result_elements and result_attributes inside the function run. Instead, you could save inside your State the JSX attribute that we found, and the role name we found .e.g (AnyJsxAttribute, TokenText). And we search for the "concept"/"element" that we need inside the diagnostic function. This will remove all those string allocations that aren't need. Like this, we will only allocate the string that we need.

@fujiyamaorange
Copy link
Contributor Author

@ematipico
Thank you for the comments!
I modified some points. 👍

Comment on lines 469 to 470
> 8 │ <div role="button" ></div>
│ ^^^^^^^^^^^^^
│ ^^^^
Copy link
Member

Choose a reason for hiding this comment

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

The range here is incorrect. It should highlight the whole attribute as before

Comment on lines +462 to +464
! The elements with the following roles can be changed to the following elements:
<button>
<input type="button">
Copy link
Member

Choose a reason for hiding this comment

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

If we're able to suggest possible alternatives like in this case, why do we keep showing the loooong list at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing.
I modified suggestion messages in 207c4f4. 👍

@ematipico ematipico force-pushed the feat-get-corresponding-element branch from 345c9fe to cd6124b Compare June 1, 2024 12:14
@github-actions github-actions bot added the A-Changelog Area: changelog label Jun 2, 2024
@ematipico ematipico merged commit 4e6a709 into biomejs:main Jun 10, 2024
12 checks passed
@fujiyamaorange fujiyamaorange deleted the feat-get-corresponding-element branch June 10, 2024 11:33
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 L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants