Skip to content

CLI docs using only markdown - #2361

Merged
Avishagp merged 2 commits into
masterfrom
feat/help-in-markdown
Nov 26, 2021
Merged

CLI docs using only markdown#2361
Avishagp merged 2 commits into
masterfrom
feat/help-in-markdown

Conversation

@JackuB

@JackuB JackuB commented Nov 12, 2021

Copy link
Copy Markdown
Contributor

Transforming CLI help to a plain markdown to improve the editing experience and portability.

@github-actions

github-actions Bot commented Nov 12, 2021

Copy link
Copy Markdown
Contributor
Warnings
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • webpack.common.ts
  • webpack.prod.ts
Messages
📖 You are modifying something in test/smoke directory, yet you are not on the branch starting with smoke/. You can prefix your branch with smoke/ and Smoke tests will trigger for this PR.

Generated by 🚫 dangerJS against 9aa0478

@Avishagp
Avishagp force-pushed the feat/help-in-markdown branch 11 times, most recently from f95eebe to 762b65c Compare November 16, 2021 11:43
@JackuB
JackuB force-pushed the feat/help-in-markdown branch from 762b65c to b583f40 Compare November 16, 2021 17:10
@Avishagp
Avishagp force-pushed the feat/help-in-markdown branch 16 times, most recently from 3446d19 to 93972c6 Compare November 22, 2021 17:29
@Avishagp
Avishagp force-pushed the feat/help-in-markdown branch from 93972c6 to 510604c Compare November 23, 2021 12:05
@Avishagp
Avishagp marked this pull request as ready for review November 23, 2021 12:22
@Avishagp
Avishagp requested review from a team as code owners November 23, 2021 12:22
@p15r
p15r removed their request for review November 23, 2021 13:23
@Avishagp
Avishagp force-pushed the feat/help-in-markdown branch 5 times, most recently from faa6518 to 6456e9a Compare November 24, 2021 11:43

@aron aron left a comment

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.

Added some suggestions inline to fix broken links and some minor formatting inconsistencies and broken likes but otherwise looks good to me 👍

Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-docs/iac-examples.md Outdated
Comment thread help/commands-md/snyk-iac.md Outdated
Comment thread help/commands-md/snyk-help.md Outdated
Comment thread help/commands-md/snyk-help.md Outdated

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good. This will reduce a lot of PR friction around updating docs. 🎉

I've left a bunch of improvements we can make, but I don't think we should block this PR as it's greatly improving what we currently have. I'm mostly leaving comments to go back to later as this PR is huge.

Let's discuss later.

Comment thread webpack.prod.ts Outdated

@ghost ghost Nov 24, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The cause of this issue mentioned yesterday is likely because webpack.common's default export does not have an assigned type. So every usage needs the as Configuration to bypass TypeScript.

This can be avoided by assigning the type in webpack.common.

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.

Thank you so much for explaining!

Comment thread src/cli/commands/help/reflow-text.ts Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be nice to have tests for this as it's pretty complex and hard to know what each step is for.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(Repeating myself) It would be nice to have tests for this as it's pretty complex and hard to know what each step is for.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

externals is a bit of a mess, so forcing TypeScript to assume it's string[] might cause weird bugs in the future. We could move the object to its own module and type and import it here and in webpack.common. That'd make it typesafe.

Here's the type for reference:

type Externals =
	| string
	| RegExp
	| ExternalItem[]
	| (ExternalItemObjectKnown & ExternalItemObjectUnknown)
	| ((
			data: ExternalItemFunctionData,
			callback: (
				err?: Error,
				result?: string | boolean | string[] | { [index: string]: any }
			) => void
	  ) => void)
	| ((data: ExternalItemFunctionData) => Promise<ExternalItemValue>);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Other notes: we should write tests for this script. Maybe consider removing externals logic entirely as it's empty. We can bring it back if/when we need it.

Comment thread help/generator/generator.ts Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should move this execution to its own module like generate-help.ts. Having modules with both side-effects and exports will cause issues for dependant modules importing from it.

Comment thread webpack.common.ts Outdated
Comment on lines 58 to 64

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can integrate our help docs more closely with Webpack and let it do the hard work around watching, generating and bundling docs. Can't summarise the process in a comment box so let's discuss later.

Comment thread .prettierignore Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why don't we want Prettier to format -docs? 🤔

Comment thread package.json Outdated

@ghost ghost Nov 24, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think we need this "directories" key at all. It's just CommonJS metadata that nothing seems to be using.

https://docs.npmjs.com/cli/v8/configuring-npm/package-json#directories

http://wiki.commonjs.org/wiki/Packages/1.0

Less maintenance for us if we remove it.

@Avishagp
Avishagp force-pushed the feat/help-in-markdown branch 5 times, most recently from 2705b3e to c5ccc61 Compare November 26, 2021 14:47
@Avishagp
Avishagp force-pushed the feat/help-in-markdown branch from c5ccc61 to 9aa0478 Compare November 26, 2021 15:07
@Avishagp
Avishagp merged commit fac3e46 into master Nov 26, 2021
@Avishagp
Avishagp deleted the feat/help-in-markdown branch November 26, 2021 16:09
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.

3 participants