Skip to content

feat(cli/doc): Support doc for runtime built-ins #4635

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 9, 2020
Merged

feat(cli/doc): Support doc for runtime built-ins #4635

merged 5 commits into from
Apr 9, 2020

Conversation

nayeemrmn
Copy link
Contributor

@nayeemrmn nayeemrmn commented Apr 5, 2020 •
edited
Loading

cc @lucacasonato @bartlomieju

Equivalent to deno doc https://github.com/denoland/deno/releases/download/v0.39.0/lib.deno.d.ts:

Updated

Show documentation for runtime built-ins:
    deno doc
    deno doc --builtin Deno.Listener

Old

Show documentation for runtime built-ins:
    deno doc
    deno doc $ Deno.Listener

bartlomieju
bartlomieju reviewed Apr 6, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, sans $ - I leave it to @ry to decide what symbol should be used

@ry
Copy link
Member

ry commented Apr 7, 2020 •
edited
Loading

The $ is very strange. We need another solution.

How about deno doc --runtime ?

@nayeemrmn
Copy link
Contributor Author

@ry Can we use an explicit empty value? It's reasonable next to the default case.

Show documentation for runtime built-ins:
    deno doc
    deno doc "" Deno.Listener

If not, I can't find a way of conditioning the first positional argument on --runtime not being given.

@ry
Copy link
Member

ry commented Apr 7, 2020

Can we use an explicit empty value?

No, that's also difficult to read. I think a flag like --runtime or --builtin would be more appropriate.

@ry
Copy link
Member

ry commented Apr 7, 2020

--global ?

@lucacasonato
Copy link
Member

I think --runtime makes sense.

@nayeemrmn
Copy link
Contributor Author

nayeemrmn commented Apr 7, 2020 •
edited
Loading

Using a clap group for --builtin and source_file, I got

USAGE:
    deno doc [OPTIONS] <--builtin|source_file> [source_file]

OPTIONS:
    -h, --help                        Prints help information
        --json                        Output documentation in JSON format.
    -L, --log-level <log-level>       Set log level [possible values: debug, info]
    -q, --quiet                       Suppress diagnostic output
    -r, --reload=<CACHE_BLACKLIST>    Reload source code cache (recompile TypeScript)
        --builtin                     Show documentation for runtime built-ins.

The second source_file should be filter... seems like a clap bug.
Using a small hack, I got the desired usage string for some reason:

USAGE:
    deno doc [OPTIONS] <--builtin|source_file> [filter]

But it still doesn't parse right. deno doc --builtin Deno.Listener binds Deno.Listener to source_file instead of filter.

@nayeemrmn nayeemrmn force-pushed the deno-doc-built-ins branch from 418d7f6 to f04a02b Compare April 8, 2020 13:04
@nayeemrmn nayeemrmn changed the title feat(cli/doc): Support doc for runtime built-ins WIP: feat(cli/doc): Support doc for runtime built-ins Apr 8, 2020
@nayeemrmn nayeemrmn mentioned this pull request Apr 8, 2020
Open
@nayeemrmn nayeemrmn force-pushed the deno-doc-built-ins branch from f04a02b to 20f6845 Compare April 9, 2020 10:56
@nayeemrmn nayeemrmn changed the title WIP: feat(cli/doc): Support doc for runtime built-ins feat(cli/doc): Support doc for runtime built-ins Apr 9, 2020
@nayeemrmn
Copy link
Contributor Author

Opened clap-rs/clap#1794, but we have to land this workaround for now (see todo).

@bartlomieju Please review.

bartlomieju
bartlomieju approved these changes Apr 9, 2020
View reviewed changes
cli/flags.rs
Comment on lines +826 to +830
// TODO(nayeemrmn): Make `--builtin` a proper option. Blocked by
// https://github.com/clap-rs/clap/issues/1794. Currently `--builtin` is
// just a possible value of `source_file` so leading hyphens must be
// enabled.
.setting(clap::AppSettings::AllowLeadingHyphen)
Copy link
Member

Choose a reason for hiding this comment

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

👍

cli/doc/parser.rs
Comment on lines +190 to +194
pub fn parse_source(
&self,
file_name: &str,
source_code: &str,
) -> Result<Vec<DocNode>, ErrBox> {
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly this method is needed only to parse given source code without using loader, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was satisfied previously by the old parse() signature before reexport support.

@bartlomieju bartlomieju merged commit 71ac552 into denoland:master Apr 9, 2020
@nayeemrmn nayeemrmn deleted the deno-doc-built-ins branch April 9, 2020 13:45
@epage epage mentioned this pull request Dec 6, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nayeemrmn @ry @lucacasonato @bartlomieju