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

Add Yarn-related rules #39

Merged
merged 18 commits into from
Jan 4, 2024
Merged

Add Yarn-related rules #39

merged 18 commits into from
Jan 4, 2024

Conversation

mcmire
Copy link
Collaborator

@mcmire mcmire commented Dec 22, 2023

For a given project, we want to ensure that it is using the same Yarn version as the module template, that Yarn is configured exactly the same way as in the module template, and that all of the Yarn-related files are present, including the Yarn "binary" in .yarn/releases and the plugins in .yarn/plugins. We also want to ensure that the version we recommend users install in the README matches the same version as the module template.

To accomplish this, this commit adds six new rules:

  • all-yarn-modern-files-conform
    • This checks that .yarnrc.yml is present and matches the module template, and any files in the template's .yarn/plugins and .yarn/releases directories are present in the project and match.
  • classic-yarn-config-file-absent
    • This checks that .yarnrc, which is the configuration file for Yarn Classic, is not present.
  • package-manager-conforms
    • This checks that the packageManager field in the project's package.json is the same as that of the module template's.
  • readme-lists-correct-yarn-version
    • This checks that the Yarn version in the project's README is the same as that of the module template's.
  • require-readme
    • This checks that the project has a README.
  • require-valid-package-manifest
    • This checks the project has a package.json and that the JSON-parsed content of this file matches a known shape.

Fixes #11.
Fixes #23.

@mcmire mcmire requested a review from a team as a code owner December 22, 2023 18:08
@mcmire mcmire mentioned this pull request Dec 22, 2023
For a given project, we want to ensure that it is using the same Yarn
version as the module template, that Yarn is configured exactly the same
way as in the module template, and that all of the Yarn-related files
are present, including the Yarn "binary" in `.yarn/releases` and the
plugins in `.yarn/plugins`. We also want to ensure that the version we
recommend users install in the README matches the same version as the
module template.

To accomplish this, this commit adds six new rules:

- `all-yarn-modern-files-conform`
  - This checks that `.yarnrc.yml` is present and matches the module
    template, and any files in the template's `.yarn/plugins` and
    `.yarn/releases` directories are present in the project and match.
- `classic-yarn-config-file-absent`
  - This checks that `.yarnrc`, which is the configuration file for Yarn
    Classic, is not present.
- `package-manager-conforms`
  - This checks that the `packageManager` field in the project's
    `package.json` is the same as that of the module template's.
- `readme-lists-correct-yarn-version`
  - This checks that the Yarn version in the project's README is the
    same as that of the module template's.
- `require-readme`
  - This checks that the project has a README.
- `require-valid-package-manifest`
  - This checks the project has a `package.json` and that the
    JSON-parsed content of this file matches a known shape.
src/misc-utils.ts Outdated Show resolved Hide resolved
src/main.test.ts Outdated Show resolved Hide resolved
src/rules/helpers.test.ts Outdated Show resolved Hide resolved
* @param ruleExecutionArguments - Rule execution arguments.
* @returns Either a successful or failed result.
*/
export async function fileConforms(
Copy link
Member

Choose a reason for hiding this comment

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

These last three functions appear to be missing tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests added here: b683cd0

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks great! Left a few comments, mostly minor suggestions or questions though

src/rules/helpers.ts Outdated Show resolved Hide resolved
/Install \[Yarn [^[\]]+\]\([^()]+\)/u,
);
if (!match?.[0]) {
throw new Error(
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the process by throwing an error! The consuming project won't be able to analyse because of the issue in template. How about displaying an non-breaking error (say fail) / warning instead?

Copy link
Collaborator Author

@mcmire mcmire Jan 4, 2024

Choose a reason for hiding this comment

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

My thought was a rule should only fail if the project does not meet whatever condition is being checked. In this case, we can't even run the rule, because the thing we're using as a point of comparison doesn't exist in the module template. I felt like this was a bug in the rule and not a deficiency in the project itself, so throwing an error seemed more appropriate so that it stood out and hopefully we could be notified of the problem and fix the bug.

That said, you are totally right that throwing an error would prevent other rules from being run, which doesn't seem ideal. I would probably need to fix this in execute-rules.ts.

Printing a warning (and returning a passing result) seems like a good solution for now, and then I can follow this up with another PR to properly address this. I've made this change here: ae3da2c

Copy link
Collaborator Author

@mcmire mcmire Jan 4, 2024

Choose a reason for hiding this comment

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

Sorry — reverted in 1b404db and d2afd04. For now, throwing is more accurate than printing a warning and then passing, which could be confusing without context as to why that's happening. I will still make a ticket to fix execute-rules.ts, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah! It's tricky here!!
My point is that, even when there's a error we should gracefully convey to the user rather than aborting the running process. When we see here, there's a issue with the template project rather than the issue in the tool itself!! It would be great, if we can find a way to differentiate that to the user.

src/rules/require-valid-package-manifest.ts Show resolved Hide resolved
@kanthesha
Copy link
Contributor

Overall looks neat! Left few minor suggestions / comments.

@mcmire
Copy link
Collaborator Author

mcmire commented Jan 4, 2024

I believe I've addressed all of the feedback now.

Gudahtt
Gudahtt previously approved these changes Jan 4, 2024
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

src/rule-helpers.test.ts Show resolved Hide resolved
@mcmire mcmire merged commit e456939 into main Jan 4, 2024
22 checks passed
@mcmire mcmire deleted the add-yarn-rules branch January 4, 2024 23:12
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.

module-lint: Make sure package.json exists Add checks for Yarn compliance
3 participants