-
Notifications
You must be signed in to change notification settings - Fork 146
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
Do not warn about nesting css selectors when firefoxStrictMinVersion is >= 117 #5164
Do not warn about nesting css selectors when firefoxStrictMinVersion is >= 117 #5164
Conversation
a621607
to
cec0430
Compare
…is >= 117 Co-authored-by: JimKnoxx <imknoxx@knoxx.media>
cec0430
to
23420b2
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5164 +/- ##
=======================================
Coverage 98.73% 98.74%
=======================================
Files 54 54
Lines 2932 2937 +5
Branches 893 896 +3
=======================================
+ Hits 2895 2900 +5
Misses 37 37 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the PR. Here is some feedback, thanks!
src/parsers/manifestjson.js
Outdated
* @property {string} version | ||
* @property {string} firefoxMinVersion | ||
* @property {Set<string>} experimentApiPaths | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can that be either omitted or moved near getMetadata()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it down. I added JSDoc, because it helped me a great deal understand what structure I can rely on. I thought it might help future devs aswell, that do not know the internals yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against but I think I'd rather see that added in a different PR.
* @param {string|Buffer|import('stream').Readable} contents | ||
* @param {string} filename | ||
* @param {ScannerOptions} options | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That kinda looks like typescript stuff, is this really needed as part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kind of JSDoc comments are the compromise of having types without using typescript. It helps documenting interfaces, allowing IDEs with code suggestions and helping new devs to faster move around foreign code. I typed parts where I struggled with the given interfaces and needed to dig deeper and documented it right away, so others don't have to. Is this kind of type-documenting style unwelcome in this repository? I can definitely just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see other comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I requested some more changes but this is looking great.
Co-authored-by: William Durand <will+git@drnd.me>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks!
Fixes #5102
Since Firefox Version 117 CSS selector nesting is supported.
This is not reflected in the
rules/css/invalidNestingSelectors.js
of the addons-linter.In this changeset I introduced a check against the
firefoxMinStrictVersion
, orstrict_min_version
which is specified in themanifest.json
.Fixes #ISSUENUM
at the top of your PR.┆Issue is synchronized with this Jira Task