-
Notifications
You must be signed in to change notification settings - Fork 25
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: add no-empty-fields rule #741
base: main
Are you sure you want to change the base?
Conversation
@JoshuaKGoldberg I completed code and test cases. Maybe the code should be optimized. Please review it when you have time. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
==========================================
- Coverage 98.01% 97.82% -0.19%
==========================================
Files 18 19 +1
Lines 1408 1150 -258
Branches 133 139 +6
==========================================
- Hits 1380 1125 -255
+ Misses 28 25 -3 ☔ 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.
This is a solid start, thanks for working on it @chouchouji! 🚀
I left some comments on making it more comprehensive in feature reporting and more targeted in how it fixes code. Please let me know if anything is unclear. 🙂
Note: some of the comments might be made irrelevant based on other comments. For example, if you end up refactoring the fix
function(s) to not use variables, the suggestion to put fix
-specific variables inside the fix
function(s) wouldn't make sense anymore.
By the way:
It's customary to put a message like
I edited that in just now. You can read more here: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword. Cheers! 🙂 |
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.
Nice progress! I just made an additional observation about the JSON.parse
@michaelfaith @JoshuaKGoldberg Thanks, I had updated code. It seems that I am working in a right direction. Please check this video. 20250120-233127.mp4 |
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.
Much cleaner! Looks great. Left one minor (non-blocking) nit. Also, want to give @JoshuaKGoldberg a chance to take another look before calling it done. I can take care of creating the doc page that CI is blocking for, unless you'd like to take a stab at that too.
src/rules/no-empty-fields.ts
Outdated
JSONArrayExpression(node: JsonAST.JSONArrayExpression) { | ||
if ( | ||
!node.elements.length && | ||
node.parent.type === "JSONProperty" |
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.
[Testing] Removing the && node.parent.type === "JSONProperty"
logic doesn't fail any tests. So one of the following is true:
- The logic is necessary (
node.parent
doesn't always have.type === "JSONProperty"
), and test cases should be added to exercise this logic - The logic is unnecessary (
node.parent
always has.type === "JSONProperty"
), so should be refactored to be unnecessary, removed, or replaced with a type assertion
This brings up a couple of interesting test cases actually:
// package.json
{
"some-custom-array": [ [] ],
"some-custom-object": [ {} ],
}
What should the rule do in those cases?
My gut instinct is that it should report on them by default, because that's just a weird thing to do - even in custom fields. Similar to #741 (comment): if someone has a tool-specific use case for an empty field, then they can always file a feature request for the rule to add an allowlist or something like that.
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 think they are two cases.
- It includes key and value, e.g.
"xxx": [], "yyy": {}
. The array or object have one key - It includes value only, e.g.
"xxx": [ {}, [] ]
. The array or object have no key
So I should update my code like this code:
JSONArrayExpression(node: JsonAST.JSONArrayExpression) {
if (!node.elements.length) {
report(
context,
node.parent.type === "JSONProperty" ? node.parent : node,
);
}
},
You can check the video
20250121-223919.mp4
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.
Hmm, that's a good question. I do wonder if there might be a legitimate use case for this. Such as resetting some tool config? I.e. some tool has a default config that's applied in a root level package.json, and a nested package.json wants to clear the config at the lower level? Not sure how likely that is, but, to your point, we could always take care of it, and see if anyone brings an actual use-case forward.
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.
Hmm, that's a good question. I do wonder if there might be a legitimate use case for this. Such as resetting some tool config? I.e. some tool has a default config that's applied in a root level package.json, and a nested package.json wants to clear the config at the lower level? Not sure how likely that is, but, to your point, we could always take care of it, and see if anyone brings an actual use-case forward.
Maybe we can consider a variable that defined in schema to choose what to keep and what to remove for user?
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.
In this first iteration, I'd advocate for keeping it simple. And if we can't articulate a substantive real-world use-case, then I think @JoshuaKGoldberg 's suggestion of covering it for now, until someone brings a valid usecase forward makes sense. It's easier / less intrusive to relax a rule than it is to make a rule more strict. So, if you already have the empty nested case working, let's go with that.
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.
In this first iteration, I'd advocate for keeping it simple. And if we can't articulate a substantive real-world use-case, then I think @JoshuaKGoldberg 's suggestion of covering it for now, until someone brings a valid usecase forward makes sense. It's easier / less intrusive to relax a rule than it is to make a rule more strict. So, if you already have the empty nested case working, let's go with that.
I agree with you. Can you review this latest code? I have the empty nested case working. Please tell me if something need to improve.
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.
Progress 🚀
docs/rules/no-empty-fields.md
Outdated
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.
[Docs] Could you please write up descriptions and examples for this file, similar to other rule docs files?
(noting Michael's comment, that works too if you prefer)
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 think I can have a try.
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.
@chouchouji if you end up having any difficulty, I'm happy to help with 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.
@chouchouji if you end up having any difficulty, I'm happy to help with it.
Thanks, I will write docs after completing this rule logic.
PR Checklist
status: accepting prs
Overview