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.

Sign up for GitHub

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

Jump to bottom

feat: add no-empty-fields rule #741

Merged
merged 23 commits into from
Jan 23, 2025
Merged

feat: add no-empty-fields rule #741

merged 23 commits into from
Jan 23, 2025
+469 −0

Conversation

chouchouji
Copy link
Contributor

@chouchouji chouchouji commented Jan 18, 2025 •
edited
Loading

PR Checklist

Overview

@chouchouji chouchouji marked this pull request as draft January 18, 2025 06:19
@chouchouji chouchouji marked this pull request as ready for review January 18, 2025 08:14
@chouchouji
Copy link
Contributor Author

@JoshuaKGoldberg I completed code and test cases. Maybe the code should be optimized. Please review it when you have time.

@codecov Codecov
Copy link

codecov bot commented Jan 19, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.88%. Comparing base (1167f5e) to head (8bf35d9).
Report is 20 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #741      +/-   ##
==========================================
- Coverage   98.01%   97.88%   -0.13%     
==========================================
  Files          18       19       +1     
  Lines        1408     1180     -228     
  Branches      133      142       +9     
==========================================
- Hits         1380     1155     -225     
+ Misses         28       25       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoshuaKGoldberg
JoshuaKGoldberg requested changes Jan 19, 2025
View reviewed changes
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment •
edited
Loading

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.

src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
src/utils/predicates.ts Outdated Show resolved Hide resolved
src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Jan 19, 2025
@JoshuaKGoldberg
Copy link
Owner

JoshuaKGoldberg commented Jan 19, 2025 •
edited
Loading

By the way:

* [x]  Addresses an existing open issue: fixes #000
* [x]  That issue was marked as [`status: accepting prs`](https://github.com/JoshuaKGoldberg/eslint-plugin-package-json/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22)
* [ ]  Steps in [CONTRIBUTING.md](https://github.com/JoshuaKGoldberg/eslint-plugin-package-json/blob/main/.github/CONTRIBUTING.md) were taken

## Overview
## Issue
close #683

It's customary to put a message like closes #... or fixes #... inside the checklist. Like:

* [x]  Addresses an existing open issue: close #683

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! 🙂

@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Jan 19, 2025
Closed
3 tasks
michaelfaith
michaelfaith reviewed Jan 20, 2025
View reviewed changes
Copy link
Collaborator

@michaelfaith michaelfaith left a 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

src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
@chouchouji
Copy link
Contributor Author

chouchouji commented Jan 20, 2025 •
edited
Loading

@michaelfaith @JoshuaKGoldberg Thanks, I had updated code. It seems that I am working in a right direction. Please check this video.

20250120-233127.mp4

@michaelfaith michaelfaith removed the status: waiting for author Needs an action taken by the original poster label Jan 20, 2025
michaelfaith
michaelfaith reviewed Jan 20, 2025
View reviewed changes
Copy link
Collaborator

@michaelfaith michaelfaith left a 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 Show resolved Hide resolved
JoshuaKGoldberg
JoshuaKGoldberg requested changes Jan 21, 2025
View reviewed changes
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Progress 🚀

src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
src/rules/no-empty-fields.ts Outdated Show resolved Hide resolved
docs/rules/no-empty-fields.md Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Jan 21, 2025
@chouchouji
Copy link
Contributor Author

@JoshuaKGoldberg @michaelfaith Thanks for your help. I had updated code. Please check this video.

20250122-200758.mp4

michaelfaith
michaelfaith reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@michaelfaith michaelfaith left a comment

Choose a reason for hiding this comment

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

Nice work on the refactor. The rule logic is looking good to me. I think fleshing out the documentation page might be all that's left, which I'm happy to help with, if you want.

@chouchouji
Copy link
Contributor Author

Nice work on the refactor. The rule logic is looking good to me. I think fleshing out the documentation page might be all that's left, which I'm happy to help with, if you want.

Thanks. I have no experience about docs. It would be best if you could write it.

michaelfaith
michaelfaith reviewed Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@michaelfaith michaelfaith left a comment

Choose a reason for hiding this comment

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

Here's a recommendation for the rule documentation. What do you think?

docs/rules/no-empty-fields.md Show resolved Hide resolved
@chouchouji
Copy link
Contributor Author

Here's a recommendation for the rule documentation. What do you think?

Thanks. I think it is very good. I had updated docs. Please review it if you have time.

michaelfaith
michaelfaith reviewed Jan 23, 2025
View reviewed changes
Copy link
Collaborator

@michaelfaith michaelfaith left a comment

Choose a reason for hiding this comment

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

Great. Nice work on this. I'll let @JoshuaKGoldberg give it another once over, but I think it's in a good spot.

One thing I'll call out is that technically this change is a breaking one, since we're adding a new rule to the recommended config. With the plugin being pre-1.0 and still forming its initial public api, I don't think that's a problem, but just mentioning it for general awareness.

@JoshuaKGoldberg
Copy link
Owner

Yeah the joy of being on a 0.x major is that semver explicitly allows us to add these "breaking" changes whenever we want 🚀. https://semver.org/#spec-item-4:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

Obviously we're not going to go too wild with it. I personally try to keep to minor versions, rather than patch versions, for things that would be breaking in a >=1.x.

JoshuaKGoldberg
JoshuaKGoldberg approved these changes Jan 23, 2025
View reviewed changes
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

💯 this is looking great, thanks!

JoshuaKGoldberg
JoshuaKGoldberg reviewed Jan 23, 2025
View reviewed changes
src/rules/no-empty-fields.ts

if (tokenFromCurrentLine?.value === ",") {
yield fixer.remove(tokenFromCurrentLine);
}
Copy link
Owner

Choose a reason for hiding this comment

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

[Non-Actionable] This logic is another example of something that'll be simplified by #750. I'll go ahead and merge this PR now and factor the changes into #750.

@JoshuaKGoldberg JoshuaKGoldberg merged commit e57765b into JoshuaKGoldberg:main Jan 23, 2025
12 of 13 checks passed
@github-actions GitHub Actions
Copy link

🎉 This is included in version v0.21.0 🎉

The release is available on:

Cheers! 📦🚀

@chouchouji chouchouji deleted the feat-rule branch January 23, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfaith michaelfaith michaelfaith left review comments

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Rule to flag empty arrays/objects that do nothing
3 participants
@chouchouji @JoshuaKGoldberg @michaelfaith