Skip to content

Add normalize-yaml package to normalize YAML consistently#5066

Merged
aduth merged 5 commits intomainfrom
aduth-js-normalize-yaml
May 18, 2021
Merged

Add normalize-yaml package to normalize YAML consistently#5066
aduth merged 5 commits intomainfrom
aduth-js-normalize-yaml

Conversation

@aduth
Copy link
Copy Markdown
Contributor

@aduth aduth commented May 17, 2021

Supersedes #5028

Like #5028, this resolves an issue where running make normalize_yaml can produce local changes due to differences in how the YAML is formatted, and due to lack of support for comments in the current YAML normalizing scripts.

Compared to #5028, this approach implements normalization using a new internal Yarn package, with the benefit of:

  • Supporting comments in YAML files
  • Being ~2x faster
  • Being self-contained, as opposed to 3 separate tasks: sorting keys via i18n-tasks normalize, formatting content via scripts/normalize-yaml, and normalizing output via prettier

For review, consider the commit in isolation: 0143506

aduth added 2 commits May 17, 2021 16:01
**Why**:

- Preserve YAML comments
- Faster
- Self-contained
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +1 to +14
{
"name": "@18f/identity-normalize-yaml",
"private": true,
"version": "1.0.0",
"type": "module",
"bin": {
"normalize-yaml": "./cli.js"
},
"dependencies": {
"prettier": "^2.2.1",
"smartquotes": "^2.3.2",
"yaml": "^2.0.0-5"
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there anything we can do that marks this as "please don't import me into the browser, I'm just meant for development"?

Copy link
Copy Markdown
Contributor Author

@aduth aduth May 18, 2021

Choose a reason for hiding this comment

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

is there anything we can do that marks this as "please don't import me into the browser, I'm just meant for development"?

It's not something I've seen. Usually it's a judgment call from the consumer, and bundlers would often go out of their way to support packages written for a Node setting to be bundled for a browser (e.g. Webpack). This package could run in a browser, AFAICT, even if the bundle size would be pretty large. If we really wanted to, we could set a browser property here that points to a no-op, but I don't really think it's necessary.

aduth and others added 3 commits May 18, 2021 08:59
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
/** @type {Record<string,any>=} */
const prettierConfig = prettier.resolveConfig.sync(process.cwd());

const files = process.argv.slice(2);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Toyed with supporting piped input to avoid the multi-step make update_country_dialing_codes. Works, but keeping as-is since it's a bit simpler.

diff --git a/Makefile b/Makefile
index b9edbdcc9..61c96ae61 100644
--- a/Makefile
+++ b/Makefile
@@ -87,8 +87,7 @@ lint_optimized_assets: optimize_assets
 	(! git diff --name-only | grep "\.svg$$") || (echo "Error: Optimize assets using 'make optimize_assets'"; exit 1)
 
 update_country_dialing_codes:
-	bundle exec ./scripts/pinpoint-supported-countries > config/country_dialing_codes.yml
-	yarn normalize-yaml config/country_dialing_codes.yml
+	bundle exec ./scripts/pinpoint-supported-countries | yarn run --silent normalize-yaml > config/country_dialing_codes.yml
 
 check_asset_strings:
 	find ./app/javascript -name "*.js*" | xargs ./scripts/check-assets
diff --git a/app/javascript/packages/normalize-yaml/cli.js b/app/javascript/packages/normalize-yaml/cli.js
index 9c67362d4..8b0afbb2a 100755
--- a/app/javascript/packages/normalize-yaml/cli.js
+++ b/app/javascript/packages/normalize-yaml/cli.js
@@ -4,16 +4,26 @@ import { join } from 'path';
 import prettier from 'prettier';
 import normalize from './index.js';
 
-const { readFile, writeFile } = fsPromises;
-
 /** @type {Record<string,any>=} */
 const prettierConfig = prettier.resolveConfig.sync(process.cwd());
 
-const files = process.argv.slice(2);
-Promise.all(
-  files.map(async (relativePath) => {
-    const absolutePath = join(process.cwd(), relativePath);
-    const content = await readFile(absolutePath, 'utf8');
-    await writeFile(absolutePath, normalize(content, prettierConfig));
-  }),
-);
+if (process.stdin.isTTY) {
+  const { readFile, writeFile } = fsPromises;
+  const files = process.argv.slice(2);
+  Promise.all(
+    files.map(async (relativePath) => {
+      const absolutePath = join(process.cwd(), relativePath);
+      const content = await readFile(absolutePath, 'utf8');
+      await writeFile(absolutePath, normalize(content, prettierConfig));
+    }),
+  );
+} else {
+  (async () => {
+    let content = '';
+    for await (const chunk of process.stdin) {
+      content += chunk.toString();
+    }
+
+    process.stdout.write(normalize(content, prettierConfig));
+  })();
+}

@aduth aduth merged commit 65e1b5d into main May 18, 2021
@aduth aduth deleted the aduth-js-normalize-yaml branch May 18, 2021 19:12
aduth added a commit that referenced this pull request Aug 19, 2021
**Why**: Makes distance_of_time_in_words less error-prone and verbose, and aligns to library documented defaults. Previously only limited this way due to limitations of YAML normalization resolved by #5066.
aduth added a commit that referenced this pull request Aug 19, 2021
**Why**: Makes distance_of_time_in_words less error-prone and verbose, and aligns to library documented defaults. Previously only limited this way due to limitations of YAML normalization resolved by #5066.
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.

2 participants