Support checksyncrc files #812

somewhatabstract · 2021-11-22T02:30:24Z

Summary:

This adds support for configuration files so that we don't have to supply all options on the command line.
It also adds test coverage for our output sink class and some other light tweaks.

This is the last piece for CheckSync v3! (Though I may try addressing #666)

Note to reviewers:

Much of this diff is test and snapshots updates. I recommend filtering out .snap files and skipping over _test.js files to look at the JSON and JS file changes.

Fixes #664

Test plan:

yarn test
yarn flow

codecov · 2021-11-22T02:31:10Z

Codecov Report

Merging #812 (037a0fc) into main (772deb8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #812    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           28        34     +6     
  Lines          455       682   +227     
  Branches        85       145    +60     
==========================================
+ Hits           455       682   +227

Impacted Files	Coverage Δ
src/exit-codes.js	`100.00% <ø> (ø)`
src/get-files.js	`100.00% <ø> (ø)`
src/marker-parser.js	`100.00% <ø> (ø)`
src/types.js	`100.00% <ø> (ø)`
src/ancesdir-or-currentdir.js	`100.00% <100.00%> (ø)`
src/cli.js	`100.00% <100.00%> (ø)`
src/default-options.js	`100.00% <100.00%> (ø)`
src/determine-options.js	`100.00% <100.00%> (ø)`
src/file-reference-logger.js	`100.00% <100.00%> (ø)`
src/find-configuration-file.js	`100.00% <100.00%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7805955...037a0fc. Read the comment docs.

somewhatabstract · 2021-11-22T02:32:22Z

.checksyncrc.json

+{
+    "$schema": "./src/checksync.schema.json",
+    "$comment": "This is the config file for our integration tests. By not specifying it directly, we also test configuration file discovery.",
+    "autoFix": false,
+    "comments": [
+        "//",
+        "#",
+        "{/*"
+    ],
+    "dryRun": false,
+    "excludeGlobs": [
+        "**/excluded/**"
+    ],
+    "ignoreFiles": [
+        "**/ignore-file.txt"
+    ],
+    "json": false
+}


An example config file for checksync. Note that the schema can be specified at the top of the file as per JSON Schema specifications (the schema is shipped with the tool).

Note that folks don't need to include this - their configs will still be validated against this schema, regardless. However, for tools like VSCode, this tells it what schema to use and provides autocomplete/intellisense accordingly.

somewhatabstract · 2021-11-22T02:33:18Z

flow-typed/hyperjump-json-schema.js

@@ -0,0 +1,55 @@
+declare module "@hyperjump/json-schema" {


Pieced this together from typescript and reading the code.

somewhatabstract · 2021-11-22T02:35:24Z

src/checksync.schema.json

@@ -0,0 +1,79 @@
+{
+    "$schema": "https://json-schema.org/draft/2020-12/schema",


This is the JSON Schema definition file against which our config files will be compared.

somewhatabstract · 2021-11-22T02:36:54Z

src/cli.js

+    // NOTE: minimist treats `no` on the front of a known flag
+    // as a flag inversion of the none-`no` version.


I learned this the hard way. Nice default feature, if you know it exists.

somewhatabstract · 2021-11-22T02:40:03Z

src/load-configuration-file.js

+        const schema = await JsonSchema.get("file://./checksync.schema.json");
+
+        log.verbose(() => `Validating configuration`);
+        const validation = await JsonSchema.validate(


The validation does include details of the actual errors, but it requires some work to turn that into human readable info that can be acted upon, so I'm going to not do that for now.

somewhatabstract · 2021-11-22T03:04:03Z

src/main.js

+export {default as checkSync} from "./check-sync.js";
+export {default as loadConfigurationFile} from "./load-configuration-file.js";


Checksync can now be used as an API. Should help with eslint integration, @kevinbarabash

This should improve performance of our eslint rule significantly.

somewhatabstract · 2021-11-22T03:04:40Z

src/string-logger.js

         */
-        const regex =
+        const rootDirRegex = new RegExp(escapeRegExp(ROOT_DIR), "g");


The verbose logging mode includes file paths - to make sure they pass, we strip the root dir off them.

somewhatabstract · 2021-11-22T03:05:22Z

src/string-logger.js

+
+        const line = `${"  ".repeat(this._groupIndent)}${args
+            .map(normalize)
+            .join("")}`.trimRight();


Trailing whitespace isn't really relevant and it can be annoying in inline snapshots when saving as eslint/prettier autofixes away empty lines. So, we trim the trailing whitespace

I've run into this elsewhere and agree, so annoying.

kevinbarabash · 2021-11-23T15:36:04Z

Thanks for adding support for config files. I'll have a look later this week.

kevinbarabash

LGTM. Supporting rc files is a great addition to checksync. I left a few comments, but nothing blocking.

.vscode/settings.json

kevinbarabash · 2021-11-29T15:20:58Z

package.json

@@ -38,6 +38,7 @@
        "@babel/preset-env": "^7.16.4",
        "@babel/preset-flow": "^7.16.0",
        "@babel/register": "^7.16.0",
+        "@hyperjump/json-schema": "^0.17.2",


Why did you pick @hyperjump/json-schema over ajv? Is it faster?

It seemed to support more recent drafts of the spec and have a better rep over all.

That and it was one of the first i landed on that did what I want (I found it via the jsonschema.org site). I could be persuaded to change it. It worked so I stopped looking.

I was curious about the speed because I tried to use ajv in the past and it was slow for the schema I had written. It could also been that my schema was overly complex. 🤷‍♂️

kevinbarabash · 2021-11-29T15:22:32Z

jest.config.js

+    // Where our coverage data does and does not come from.
+    collectCoverageFrom: ["src/**/*.js", "!src/__tests__/*.js"],
+


It's too bad jest doesn't automatically filter out test files from coverage.

src/checksync.schema.json

kevinbarabash · 2021-11-29T15:31:17Z

src/checksync.schema.json

+        "comments": {
+            "description": "An array of the strings that identify the starts of comments in the files being parsed",
+            "type": "array",
+            "items": {
+                "type": "string"
+            },
+            "uniqueItems": true,
+            "minLength": 0
+        },


It looks like json-schema support defaults. If you end up generating docs from this schema file then including defaults might be useful. I'm not sure how schema parsing libraries deal with defaults. It would be nice if you could pass it an empty JSON object and it would return an object with all the defaults.

I seem to recall including defaults in an earlier version, but I decided against it. I don't recall why. I'll check.

kevinbarabash · 2021-11-29T16:01:36Z

src/options-from-args.js

+export const optionsFromArgs = (args: minimistOutput): $Shape<Options> => {
+    const options: $Shape<Options> = {};


nit: maybe use Partial instead of $Shape.

It doesn't work for this situation.

I thought that Partial was just a better version of $Shape. Do they handle optionality differently?

No, so Partial as I understand it, says "I am a type that is any combination of the values within this type" whereas $Shape says, "I look like this type, but all my values are optional" or something along those lines.

So T can be considered equal to $Shape<T> it seems, but T is not equal to Partial<T>.

At least, that's my basic understanding.

kevinbarabash · 2021-11-29T16:03:15Z

src/options-from-args.js

+    const excludeGlobs = (args.ignore: any)?.split(";").filter((c) => !!c);
+    if (excludeGlobs != null) {
+        options.excludeGlobs = excludeGlobs;
+    }


Can ignore be false like ignoreFiles?

No, it doesn't work that way - it's either a string, or its not provided. The main reason for the ? is to work around the ambiguity of minimist typing and the fact that this argument may not be supplied.

kevinbarabash · 2021-11-29T16:04:28Z

src/options-from-args.js

+    if (args.dryRun != null) {
+        options.dryRun = !!args.dryRun;
+    }


Does minimist not provide booleans or is this just to convince Flow that these values are actually booleans?

minimist types args as boolean | string | void, I believe. So we need to coerce them to booleans for Flow to recognize we did the right thing.

src/output-sink.js

kevinbarabash · 2021-11-29T16:10:10Z

src/string-logger.js

+
+        const line = `${"  ".repeat(this._groupIndent)}${args
+            .map(normalize)
+            .join("")}`.trimRight();


I've run into this elsewhere and agree, so annoying.

somewhatabstract · 2021-12-04T19:09:47Z

src/find-configuration-file.js

+                // Could be in the working directory too, so we add a fake child
+                // to the path that ancesdir will cut off to start the search
+                // in parent locations, which will then be the cwd.


NIT: don't need to be so specific here since I added the helper method for this

somewhatabstract · 2021-12-04T19:11:52Z

src/find-configuration-file.js

+    const closestRCPath = checkSyncRcNames
+        .map((rc) => {
+            try {
+                // Could be in the working directory too, so we add a fake child
+                // to the path that ancesdir will cut off to start the search
+                // in parent locations, which will then be the cwd.
+                return path.join(ancesdirOrCurrentDir(process.cwd(), rc), rc);
+            } catch (e) {
+                return "";
+            }
+        })
+        .filter((f) => f != "")
+        .map((rcPath) => ({
+            rcPath,
+            distance: path.relative(process.cwd(), rcPath).split(path.sep)
+                .length,
+        }))
+        .reduce((acc, cur) => {
+            if (acc == null || cur.distance < acc.distance) {
+                return cur;
+            }
+            return acc;
+        }, null);


So, if we execute just checksync we want it to just work, and this caters to that. Means that even in webapp, we could just run it and it should work, even before it knows the root marker

src/get-files.js

src/help.js

somewhatabstract · 2021-12-04T19:16:57Z

src/load-configuration-file.js

+        if (!rcForValidation.$schema) {
+            rcForValidation.$schema = "file://./checksync.schema.json";
+        }


It should still validate against our schema, but their tooling will probably complain (if it supports schemas, like vscode for example).

And yes, I believe we do need to set the schema still. The idea is we're asking "validate against this set of schema" and the reference tells it which of those schema to start at. Something like that. I know I had to do a few incantations to get things working.

somewhatabstract · 2021-12-07T18:51:37Z

src/options-from-args.js

+export const optionsFromArgs = (args: minimistOutput): $Shape<Options> => {
+    const options: $Shape<Options> = {};


No, so Partial as I understand it, says "I am a type that is any combination of the values within this type" whereas $Shape says, "I look like this type, but all my values are optional" or something along those lines.

So T can be considered equal to $Shape<T> it seems, but T is not equal to Partial<T>.

At least, that's my basic understanding.

TODO: Add tests, and add validation that outputs info about bad configs

Also fixes some issues with the schema loading and makes sure that ancesdir checks the current directory

somewhatabstract self-assigned this Nov 22, 2021

somewhatabstract requested review from jeremywiebe and kevinbarabash November 22, 2021 02:30

somewhatabstract added this to the CheckSync 3.0.0 milestone Nov 22, 2021

somewhatabstract commented Nov 22, 2021

View reviewed changes

kevinbarabash approved these changes Nov 29, 2021

View reviewed changes

somewhatabstract commented Dec 7, 2021

View reviewed changes

somewhatabstract added 18 commits December 15, 2021 15:28

Add @hyperjump/json-schema package

688a09b

Add config file schema

a1199d9

Added descriptions to schema fields

e5f88e1

Implement config file search and load

560b6af

TODO: Add tests, and add validation that outputs info about bad configs

Update workspace settings to support config schema

9d61932

Make sure schema file is shipped

a8a34cd

Fix up argument use to cope with how minimist works

3bb1e63

Get config file loading working for integration tests

f3772db

Also fixes some issues with the schema loading and makes sure that ancesdir checks the current directory

Adding lots of tests

947f9ab

Add test coverage

2dc6946

Fix up paths in StringLogger

f01667b

Remove redundant code

92a2983

Put main.js back into coverage

9172b38

Exclude test files from coverage data

e1fc3b9

Really exclude test files from coverage data

aa15d3c

Add diagnostic check to see what windows says the root dir is

695b34c

Fix the root dir replacement

9c2766f

Fix new tests that have path separator issues

0ac893b

somewhatabstract added 2 commits December 15, 2021 15:29

Update README

f324961

Tighten regex per review commentary

037a0fc

somewhatabstract force-pushed the somewhatabstract/issue664 branch from b4c19d2 to 037a0fc Compare December 15, 2021 21:29

somewhatabstract merged commit 1b7b343 into main Dec 15, 2021

somewhatabstract deleted the somewhatabstract/issue664 branch December 15, 2021 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support checksyncrc files #812

Support checksyncrc files #812

somewhatabstract commented Nov 22, 2021 •

edited

codecov bot commented Nov 22, 2021 •

edited

somewhatabstract Nov 22, 2021

somewhatabstract Nov 22, 2021

somewhatabstract Nov 22, 2021

somewhatabstract Nov 22, 2021

somewhatabstract Nov 22, 2021

somewhatabstract Nov 22, 2021

kevinbarabash Nov 29, 2021

somewhatabstract Nov 22, 2021

somewhatabstract Nov 22, 2021

kevinbarabash Nov 29, 2021

kevinbarabash commented Nov 23, 2021

kevinbarabash left a comment

kevinbarabash Nov 29, 2021

somewhatabstract Nov 30, 2021

somewhatabstract Dec 4, 2021

kevinbarabash Dec 7, 2021

kevinbarabash Nov 29, 2021

kevinbarabash Nov 29, 2021

somewhatabstract Dec 4, 2021

kevinbarabash Nov 29, 2021

somewhatabstract Nov 30, 2021

kevinbarabash Dec 7, 2021

somewhatabstract Dec 7, 2021

kevinbarabash Nov 29, 2021

somewhatabstract Nov 30, 2021

kevinbarabash Nov 29, 2021

somewhatabstract Nov 30, 2021

kevinbarabash Nov 29, 2021

somewhatabstract Dec 4, 2021

somewhatabstract Dec 4, 2021

somewhatabstract Dec 4, 2021

somewhatabstract Dec 7, 2021

		@@ -0,0 +1,79 @@
		{
		"$schema": "https://json-schema.org/draft/2020-12/schema",

		// NOTE: minimist treats `no` on the front of a known flag
		// as a flag inversion of the none-`no` version.

		export {default as checkSync} from "./check-sync.js";
		export {default as loadConfigurationFile} from "./load-configuration-file.js";

		// Where our coverage data does and does not come from.
		collectCoverageFrom: ["src/*/.js", "!src/__tests__/*.js"],

		export const optionsFromArgs = (args: minimistOutput): $Shape<Options> => {
		const options: $Shape<Options> = {};

Support checksyncrc files #812

Support checksyncrc files #812

Conversation

somewhatabstract commented Nov 22, 2021 • edited

Summary:

Test plan:

codecov bot commented Nov 22, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash commented Nov 23, 2021

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract commented Nov 22, 2021 •

edited

codecov bot commented Nov 22, 2021 •

edited