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

Fix and refactor the generate-docs subcommand #788

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hellodword
Copy link
Contributor

Related to #759

Sorry it should not be the project-folder, but the src folder directly, for example:

should be ./src/test/container-templates/example-templates-sets/simple/src not ./src/test/container-templates/example-templates-sets/simple

So I rename it in order to avoid the confusing.

@hellodword hellodword requested a review from a team as a code owner March 29, 2024 06:00
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looks good to me! We already merged this new command, hence, this might be a breaking change for folks. However, I don't believe this might be utilized yet as we have not updated our *-starter repos.

Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

The package and publish commands use a positional argument for this, should we align the doc command with those. E.g. from the publish command:

devcontainer features publish <target>

Package and publish Features

Positionals:
  target
           Package and publish features at provided [target] (default is cwd), where [target] is either:
              1. A path
           to the src folder of the collection with [1..n] features.
              2. A path to a single feature that contains a d
           evcontainer-feature.json.
                                                                                      [string] [required] [default: "."]

Options:
      --help       Show help                                                                                   [boolean]
      --version    Show version number                                                                         [boolean]
  -r, --registry   Name of the OCI registry.                                               [string] [default: "ghcr.io"]
  -n, --namespace  Unique indentifier for the collection of features. Example: <owner>/<repo>        [string] [required]
      --log-level  Log level.                                      [choices: "info", "debug", "trace"] [default: "info"]

@hellodword
Copy link
Contributor Author

The package and publish commands use a positional argument for this, should we align the doc command with those. E.g. from the publish command:

devcontainer features publish <target>

Package and publish Features

Positionals:
  target
           Package and publish features at provided [target] (default is cwd), where [target] is either:
              1. A path
           to the src folder of the collection with [1..n] features.
              2. A path to a single feature that contains a d
           evcontainer-feature.json.
                                                                                      [string] [required] [default: "."]

Options:
      --help       Show help                                                                                   [boolean]
      --version    Show version number                                                                         [boolean]
  -r, --registry   Name of the OCI registry.                                               [string] [default: "ghcr.io"]
  -n, --namespace  Unique indentifier for the collection of features. Example: <owner>/<repo>        [string] [required]
      --log-level  Log level.                                      [choices: "info", "debug", "trace"] [default: "info"]

You're right, it's better, I'll switch to this

@hellodword hellodword changed the title Fix the description and rename the project-folder option of generate-… Fix and refactor the generate-docs subcommand Apr 3, 2024
@hellodword hellodword requested a review from chrmarti April 7, 2024 01:31
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Looks good, left a few comments.

@@ -191,8 +186,36 @@ async function _generateDocumentation(
}

// Write new readme
fs.writeFileSync(readmePath, newReadme);
}
fs.writeFileSync(readmePath, newReadme);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use async variant to allow other code to make progress while this is waiting for completion.


export function GenerateDocsOptions(y: Argv, collectionType: GenerateDocsCollectionType) {
return y
.options({
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep --project-folder and -p as a hidden option with a warning message in the log when used for compatibility? Since this has been available for a few weeks users might have picked it up and removing it could break things locally or in CIs.

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