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 checks for general module template adherence #6

Open
desi opened this issue Oct 11, 2023 · 3 comments
Open

Add checks for general module template adherence #6

desi opened this issue Oct 11, 2023 · 3 comments

Comments

@desi
Copy link

desi commented Oct 11, 2023

Although we have checks for specific tools — ESLint, TypeScript, etc. — it may be that we make a change to the module template and don't have a specific rule around it in module-lint. Therefore, we need a set of rules to act as catch-alls. Although these rules may not be perfect — the output of these rules may not be useful as the output of more specific rules and may not account for all of the intentional divergences from the module template — it's better than nothing.

Given this, for a given project, we want to ensure that:

  • All of JSON and YAML files in the module template exist in the project, and the project's version of the file is a superset of the module template's version
    • Essentially we want to do a recursive containment check. When comparing two objects, every property in the project's version should also exist in the module template's version and have the same value; when comparing two arrays, every item in the project's version should also exist in the module template's version; and this should be recursive.
    • This would allow for the project's version of the file to customize the JSON file in question while ensuring that it contains the same configuration that the module template specifies. For instance, a project could specify more ESLint overrides than the module template specifies.
  • All of the .js, .cjs, .mjs, and .ts files in the module template exist in the project, and the project's version of that file, when run, is a superset of the module template's version
    • This is the same check as for JSON and YAML files, except that we want to filter by .js and .cjs files that contain module.exports = or .mjs and .ts files that contain export, and we want to run the whole file within both the module template and project to obtain an object (as in, create a small Node script which imports the file and outputs the resulting object as JSON, write the JSON data to another file, and then compare JSON). If the file produces an error when run, then bail and skip to the next strategy
    • We might consider working out a way to do this in a safe way in case the script runs rm -rf or something, but perhaps that is not possible.
  • For all other files in the module template not matched by the above, ensure that they exist in the project, and that the project's version of the file starts with the template's version
    • This would account for dotfiles such as .gitignore, .gitattributes, etc. It would also account for .js, .cjs, .mts, and .ts files that aren't configuration files for some reason.
    • We want to do a "start with" check in case the project adds more configuration beyond what the module template provides.
    • Skip README.md and CHANGELOG.md since these are highly customized for each project. README.md is checked in Add README-related checks #53, and CHANGELOG.md is checked in Add changelog-related checks #48. Also skip files in src/ since they're meant to be examples.
    • Make sure to remove the "Examples" section in .github/pull_request_template.md before comparing, since it's only for the module template.
@mcmire mcmire transferred this issue from MetaMask/core Oct 12, 2023
@mcmire mcmire changed the title Whether it adheres to the module template Add check for module template adherence Oct 12, 2023
@mcmire mcmire changed the title Add check for module template adherence Add checks for module template adherence Oct 12, 2023
@mcmire mcmire changed the title Add checks for module template adherence Add checks for general module template adherence Feb 8, 2024
@Gudahtt
Copy link
Member

Gudahtt commented Feb 22, 2024

Suggestions:

  • Expand .js rule to include .ts as well, and restrict rule to just files at the root
  • For the "all other files" case, allow additional lines to be added after the contents of the template file. We can allow additional content, that might be pretty commonplace for files like gitignore for example.

@mcmire
Copy link
Collaborator

mcmire commented Feb 22, 2024

A question was raised in standup this morning about the overlap between the rule being discussed in this ticket and the existing rules we've completed and have planned. If there are already certain files we are checking via other rules, it seems that this rule would also check those files, so should we exclude them from this rule?

We could do this, but we would have to make two assumptions:

  1. We would have to assume that all rules are already using the logic presented in this ticket, i.e., they use recursive containment to check configuration instead of equality.
  2. We would also have to assume that all rules that operate on a file check the full content of the file and not a part of it.

The first assumption is valid; the existing rules do not make a containment check, but we can easily fix that. The second assumption, however, creates a problem. One of the guiding principles behind the existing rules is that they should be as bite-size and have as few responsibilities as possible. This way, a project can disable and enable individual rules to suit its needs. However, in taking this fine-tuned approach we cannot guarantee that all of the files and configuration in the module template are being covered by a rule. In other words, if a change is made to the module template in the future, it is possible there may not be a rule in module-lint which verifies that change. Therefore, we need a way to address any gaps in coverage that may exist at any given time. Hence, the rule outlined in this ticket would serve that purpose.

@mcmire
Copy link
Collaborator

mcmire commented Feb 22, 2024

I am going to suggest, based on the size of this ticket, that we hold off until we get through all of the other ones first. If the main goal of this ticket is to address potential gaps in existing rules, perhaps there is a way we can compare the rules we have with the module template and verify that everything is covered. Or perhaps having gaps is just an inevitability based on the way this tool is designed. I'll have to give this a bit more thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants