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

fix(lint/noUnknownProperty): ignore composes for noUnknownProperty #3217

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

chansuke
Copy link
Member

Summary

Closes #3000

Test Plan

@github-actions github-actions bot added L-CSS Language: CSS A-Changelog Area: changelog labels Jun 15, 2024
Copy link

codspeed-hq bot commented Jun 15, 2024

CodSpeed Performance Report

Merging #3217 will not alter performance

Comparing chansuke:fix/add-compose (6554e26) with chansuke:fix/add-compose (4cb079f)

Summary

✅ 90 untouched benchmarks

@chansuke chansuke marked this pull request as ready for review June 16, 2024 04:45
@chansuke chansuke requested a review from togami2864 June 17, 2024 02:43
@Sec-ant
Copy link
Member

Sec-ant commented Jun 17, 2024

Based on this message: #3000 (comment)

We need to modify the implementation so that it does not raise an error for the composes property if the css_modules option is enabled.

I wonder if there's a way to query the option in a rule context? We can get the file source from the context but css_modules is not a file source variant. @denbezrukov

@denbezrukov
Copy link
Contributor

denbezrukov commented Jun 17, 2024

Based on this message: #3000 (comment)

We need to modify the implementation so that it does not raise an error for the composes property if the css_modules option is enabled.

I wonder if there's a way to query the option in a rule context? We can get the file source from the context but css_modules is not a file source variant. @denbezrukov

Hm,
I’m not familiar with the analyzer infrastructure :(
I think if it’s not possible to get the option inside the rule, we can:

  1. Just ignore it and not use the option.
  2. Implement an option for the rule where we can add names to be ignored and extend it if the option is enabled.

@Sec-ant
Copy link
Member

Sec-ant commented Jun 17, 2024

I think if it’s not possible to get the option inside the rule, we can:

  1. Just ignore it and not use the option.
  2. Implement an option for the rule where we can add names to be ignored and extend it if the option is enabled.

I think ignoring the option to always accept composes is fine. We can wait for more feedbacks to decide whether to add a rule option.

@Sec-ant Sec-ant changed the title fix(lint/noUnknownProperty): add compose for noUnknownProperty fix(lint/noUnknownProperty): ignore composes for noUnknownProperty Jun 18, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@Sec-ant Sec-ant merged commit 1cdfefc into biomejs:main Jun 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add composes for noUnknownProperty.
4 participants