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.

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

Add --format option to docs command #6982

Merged
merged 7 commits into from
Dec 1, 2018
Merged

Add --format option to docs command #6982

merged 7 commits into from
Dec 1, 2018

Conversation

mniak
Copy link
Contributor

@mniak mniak commented Oct 23, 2018 •
edited
Loading

@mniak mniak mentioned this pull request Oct 23, 2018
Closed
Sija
Sija reviewed Oct 23, 2018
View reviewed changes
src/compiler/crystal/tools/doc/generator.cr Outdated
@@ -28,7 +28,7 @@ class Crystal::Doc::Generator
},
}

def initialize(@program : Program, @included_dirs : Array(String), @output_dir : String, @canonical_base_url : String?)
def initialize(@program : Program, @included_dirs : Array(String), @output_dir : String, @output_format : String, sss @canonical_base_url : String?)
Copy link
Contributor

Choose a reason for hiding this comment

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

sss? ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, typo

Sija
Sija reviewed Oct 23, 2018
View reviewed changes
src/compiler/crystal/command/docs.cr Outdated
@@ -4,7 +4,10 @@
# is in `crystal/tools/doc/`

class Crystal::Command
private VALID_OUTPUT_FORMATS = ["html", "json"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pedantic: %w(html json)

Copy link
Member

Choose a reason for hiding this comment

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

It's not really an improvement, just a different way to express this.

bcardiff
bcardiff requested changes Oct 23, 2018
View reviewed changes
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

Please avoid duplicate in the logic to grab the content of the readme file.
There is also interest in allowing the use to use other file than readme as initial doc content.

@mniak
Copy link
Contributor Author

mniak commented Oct 25, 2018

Should I skip the generation of index.json when --format=html (or missing)?

@straight-shoota
Copy link
Member

That would require to change the JavaScript for loading the docs. Currently, it loads index.json when served through HTTP and search-index.js when opened as a local file (using JSONP). But it's totally fine to use search-index.js in both cases.

crystal/src/compiler/crystal/tools/doc/html/js/_search.js

Line 528 in 7b8326f

if (script.src.indexOf("file://") == 0) {

You can tackle it right now or leave that for a new PR.

@mniak
Copy link
Contributor Author

mniak commented Oct 25, 2018 •
edited
Loading

I'd prefer to handle it in another PR

@Sija
Copy link
Contributor

Sija commented Oct 25, 2018

@mniak It would be good to add "to docs command" in the title since it's not clear now what's this option is about.

Also please change "Solves the issue" to "Fixes" or "Closes" since these two are the keywords used by GH to auto-close issues.

@mniak mniak changed the title Add --format option (#2772) Add --format option to docs command Oct 25, 2018
@Sija
Copy link
Contributor

Sija commented Oct 25, 2018

@mniak https://help.github.com/articles/closing-issues-using-keywords/

@mniak
Copy link
Contributor Author

mniak commented Oct 26, 2018

@Sija thanks for these tips

@mniak
Copy link
Contributor Author

mniak commented Oct 26, 2018

@bcardiff I tried to follow your tip avoiding duplicate the logic to grab the content of the readme file.

@RX14 RX14 requested a review from bcardiff October 27, 2018 11:36
@RX14
Copy link
Contributor

RX14 commented Nov 30, 2018 •
edited
Loading

@bcardiff ping

@RX14 RX14 added this to the 0.27.1 milestone Dec 1, 2018
@RX14 RX14 merged commit ecdb8db into crystal-lang:master Dec 1, 2018
@RX14
Copy link
Contributor

RX14 commented Dec 1, 2018

Merging this broke master :(

@bcardiff
Copy link
Member

bcardiff commented Dec 1, 2018

A conflict with the specs added during #7063

This was referenced Dec 1, 2018
Merged
Merged
@Jens0512 Jens0512 mentioned this pull request Feb 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota approved these changes

@Sija Sija Sija left review comments

@ammarfaizi2 ammarfaizi2 ammarfaizi2 approved these changes

@RX14 RX14 RX14 approved these changes

@bcardiff bcardiff bcardiff approved these changes

Assignees
No one assigned
Labels
kind:feature topic:tools:docs-generator
Projects
None yet
Milestone
0.27.1
Development

Successfully merging this pull request may close these issues.
6 participants
@mniak @straight-shoota @Sija @RX14 @bcardiff @ammarfaizi2