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(require-author): add new require-author rule #851

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

michaelfaith
Copy link
Collaborator

PR Checklist

Overview

This change adds the first or our new require- rules, and creates some foundational plumbing to make it super easy to add new require rules. I've only done author in this PR. Assuming this all looks good, we can quickly knock all the rest of the require rules in one shot.

This change adds the first or our new `require-` rules, and creates some foundational plumbing to make it super easy to add new require rules.  I've only done author in this PR.  Assuming this all looks good, we can quickly knock all the rest of the require rules in one shot.
Copy link

codecov bot commented Feb 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (336f1bf) to head (efd4263).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #851      +/-   ##
==========================================
+ Coverage   98.71%   98.77%   +0.06%     
==========================================
  Files          19       21       +2     
  Lines        1163     1220      +57     
  Branches      139      142       +3     
==========================================
+ Hits         1148     1205      +57     
  Misses         15       15              

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

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.

👍 from me on this approach, agreed it'll be handy as we scale to more properties. Nice!

Requesting changes on simplifying the two main files a bit and some more testing.

Copy link
Owner

Choose a reason for hiding this comment

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

[Style] A couple of quick thoughts here:

  • I don't think the ////* comments are worth their weight; IMO their content is inferable from the file
  • We could trim this down just a bit by using an as const and an array of tuples

WDYT of this?

const properties = [
	["author", false],
	// TODO: more to come :)
] as const;

export const rules = Object.fromEntries(
	properties.map(([propertyName, recommended]) => [
		`require-${propertyName}`,
		createRequirePropertyRule(propertyName, recommended),
	]),
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the idea of using tuples here. I changed the properties object over to that, but kept satisfies in there to enforce a type expectation. I also went with a reducer, since that felt a bit more readable / communicative.

Copy link
Owner

Choose a reason for hiding this comment

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

Also surfacing from a 1:1 conversation: I'm not against trying out more comments 😄

],
valid: [
`{ "main": "./index.js", "author": "Sophie Trudeau" }`,
`{ "author": "Jessica Moss" }`,
Copy link
Owner

Choose a reason for hiding this comment

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

[Question] What is the reference with these names? Google isn't helping me 😄

Copy link
Collaborator Author

@michaelfaith michaelfaith Feb 2, 2025

Choose a reason for hiding this comment

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

They're both members of an amazing post-rock band, Thee Silver Mt. Zion, which spun out of pioneer post-rock band Godspeed You! Black Emperor. Sophie's also a member of GYBE, and Jessica is married to Efrim who's also in both. I originally had two other people, but cspell didn't recognize the words "Reznor", "Efrim", or "Menuck", so I went with these 😊

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Feb 2, 2025
@michaelfaith michaelfaith removed the status: waiting for author Needs an action taken by the original poster label Feb 2, 2025
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.

💯 glorious!

Want to merge it whenever you're ready @michaelfaith?

@michaelfaith michaelfaith merged commit cde68da into main Feb 3, 2025
14 checks passed
@michaelfaith michaelfaith deleted the feat/require-author branch February 3, 2025 18:32
Copy link

github-actions bot commented Feb 3, 2025

🎉 This is included in version v0.22.0 🎉

The release is available on:

Cheers! 📦🚀

@michaelfaith michaelfaith mentioned this pull request Feb 3, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Bring in a require-author rule
2 participants