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

Modify cli options and API in README #445

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Tarektouati
Copy link

This PR closes issue #444

It aims to

  • Simplify usage and CLI options in the README.
  • Automate the generation of API section

TODO :

  • Modify "CLI options" section
  • Generate "API" section
  • Automate API section automation

@coveralls
Copy link

Coverage Status

coverage: 99.197%. remained the same when pulling 982c15f on Tarektouati:main into a61ef39 on open-cli-tools:main.

README.md Outdated

**Members:**

- /\*\*
Copy link
Contributor

Choose a reason for hiding this comment

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

ts-readme doesn't seem to be maintained and it doesn't seem to parse inner jsdoc like this

Copy link
Contributor

Choose a reason for hiding this comment

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

How about using typedoc?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't knew this package,
It seems pretty nice and match our use-case here.

I'll update the PR with it.

Copy link
Author

Choose a reason for hiding this comment

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

@seiwonpark I've been quite busy lately.

I've implemented typedoc let me know what do you think before I can start working on automation.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @gustavohenke @paescuj

Pretty nice job! Interfaces are documented pretty nicely but not sure if classes are. (And some other fixes are required as well)

IMHO as @gustavohenke liked tables, current table-sorted README style seems better. And there seems to be more configuration to do to automate the process. So how about just let maintainers/contributors manually update the document more readable in current stage?

I've recommended typedoc to be like some use cases (e.g. chatgpt-api, and mermaid, ... and some other open sources). typedoc could be an option but this might be too early for concurrently in this stage(because to use it like the above libraries, pretty much comments should be fixed and written to fit the typedoc configuration).

So maybe setting the issue(automating documentation generation) as <Pending> and keep @Tarektouati's current README style that contains table-sorted document (but yes update document manually..) would be enough. Or, rather implementing separate library e.g. some-library-like-jsdoc-to-readme-parser would be a nice choice.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks !

Totally agree with @seiwonpark, as I see the generation result, I'm not totally convinced.
At least, it shows where codebase should be enhanced to have a better experience with it.

Let me know if we keep "automating documentation generation" task and create other PR in that direction, or maybe just drop the related commits.

cc @gustavohenke @paescuj

README.md Outdated Show resolved Hide resolved
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

I like very much the new tables for each of the options! I think it might be possible to autogenerate those too (by inspecting yargs state).

I'm not sure I like how the TS docs generation is structured in the readme; it might be too much now. Maybe the typedoc suggestion (#445 (comment)) is good, but I haven't spent much more time looking into it. Will do once the PR is ready for review.

src/index.ts Outdated
@@ -18,11 +18,9 @@ import { LogTimings } from './flow-control/log-timings';
import { RestartProcess } from './flow-control/restart-process';
import { Logger } from './logger';

/** Logger options */
Copy link
Member

Choose a reason for hiding this comment

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

I didn't go through the .md file yet, but it's my perception (and, well, it's what we do at Canva too) that // comments are usually not picked up by tooling (doc generators and IDEs), whereas /* */ comments are, so they work mostly as a file-internal comment that doesn't need to be exposed.

The old // Logger options was here mostly to create a section (just like there are others for input handling, restarting and so on below), I don't think it adds any value being in /* */ nor is it correct as the doc for ConcurrentlyOptions

cc @paescuj just because I think this has been an unwritten rule in this codebase :)

Copy link
Author

Choose a reason for hiding this comment

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

I modified theses comments for ts-readme

they should be reverted after migrating to typedoc : (cf: #445 (comment))

Copy link
Author

@Tarektouati Tarektouati Oct 21, 2023

Choose a reason for hiding this comment

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

Reverted in af9993f

@@ -0,0 +1 @@
TypeDoc added this file to prevent GitHub Pages from using Jekyll. You can turn off this behavior by setting the `githubPages` option to false.
Copy link
Author

Choose a reason for hiding this comment

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

This file is generated by typedoc

@@ -0,0 +1,264 @@
# Class: Command
Copy link
Author

Choose a reason for hiding this comment

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

Moved to separate folder docs

Updated links in the README.md to this file as entry point for documentation

#Generate documentation for the project based on Typescript types
pnpm exec typedoc

# Typedoc generates a README.md file in the docs folder, which is not needed
Copy link
Author

Choose a reason for hiding this comment

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

I think the comment is explicit here,
I couldn't find a way to avoid typedoc to regenerate README.md file in docs folder

@Tarektouati Tarektouati marked this pull request as ready for review January 26, 2024 11:45
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.

4 participants