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

JSON output for docs #4746

Merged
merged 1 commit into from Oct 31, 2017
Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jul 25, 2017

This PR adds JSON serializers for doc items and exports all into a single JSON file with the doc generator.
This JSON data can be used for an improved search feature on the doc pages (#1792) or for tools processing (#2772) like ctags, documentation analysis, custom doc viewers etc.

It might be considered to remove all unnecessary properties for search purposes to keep the file size small. Currently it's 5.1 MB (docs dir total: 69 MB) which is not too bad and the index file should be cached on HTTP connections.

The Item.def_to_json macros are essentially the to_json part of JSON.mapping. I'd like to move them to the JSON namespace because I think it is a common use case to define only to_json methods when you neither want to parse JSON nor to automatically define instance variables as JSON.mapping does. I'd be happy to address this in a separate PR but would like some feedback on this feature.

Prerequisite for #1792
Supersedes #2777, #3762
Depends on #4772

@straight-shoota
Copy link
Member Author

Any feedback on the def_to_json macro?

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

This looks OK for the most part!

@@ -6,4 +6,68 @@ module Crystal::Doc::Item
def formatted_summary
@generator.summary(self)
end

macro def_to_json(type, properties)
Item.def_to_json(nil, {{properties}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here? I don't understand the purpose of this macro.

Copy link
Member Author

Choose a reason for hiding this comment

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

I explained the purpose in the initial post. This is basically the to-json part of JSON.mapping. It would probably be better to move this to the JSON namespace because it is generally useful to easily define a to_json method with mappings as data if you don't want the full features of JSON.mapping.

Copy link
Contributor

@RX14 RX14 Jul 31, 2017

Choose a reason for hiding this comment

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

No I meant this specific override, it just seems to me that the third macro def_to_json(type, properties) below will override this definition entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes. It should be without type, .

@straight-shoota
Copy link
Member Author

@RX14 @asterite I've refactored this PR to directly use JSON::Builder. Overall, I am surprised that it is not as much boilerplate as I had initially perceived. Besides the somewhat cumbersome arrays and nil-checks in https://github.com/crystal-lang/crystal/pull/4746/files#diff-83d7cc25038e34d9b7bdff893cdbc2a5 it is mostly okay, I think.

The previous implementation based on #4772 is available at https://github.com/straight-shoota/crystal/tree/jm-json-docs--def-to-json for comparison.

@RX14
Copy link
Contributor

RX14 commented Oct 31, 2017

Progress to get fuzzy doc search merged, probably the most anticipated feature for me.

@asterite
Copy link
Member

@straight-shoota Cool, thank you!

I know I'm being a bit more conservative about making changes (or even adding) to the standard library API, but I'd like to avoid that if possible, just to simplify the whole thing. Now this PR is mostly harmless and very good. I can see some little things being duplicated, but I think code duplication is not that bad when it's minimal. There's one Go proverb I like and always have in mind:

A little copying is better than a little dependency.

@RX14 Is fuzzy search here in this PR?

@straight-shoota
Copy link
Member Author

Yes, you're right this is probably better this way.

Fuzzy search is just waiting around the corner, I have a working prototype that will go into a new PR after this is merged.

@asterite
Copy link
Member

Hhhhhhmmmmmmmm....

I thought this was giving a JSON output for docs when you passed --format json but it seems it's generating one more file named index.json. I understand that this is for fuzzy search (maybe?) but I'm not sure it's the best way to do this.

The way I see it, we should have crystal docs be able to generate a JSON file as an output. That JSON file can then be processed to actually create HTML files and host them on a server, together with fuzzy-search and whatnot. In fact, once we do that we could get rid of crystal doc generating HTML in the compiler, just JSON is enough. Then another tool (the real doc tool) can invoke crystal doc and generate HTML. That tool can use a good Markdown library so we'll get rid of the small and incomplete Markdown module from the standard library.

@straight-shoota
Copy link
Member Author

Yeah that's likely the best way to go, but getting there will surely take some time.

Please, let's catch the easy bird and merge this for now to get fuzzy search up and running ASAP. It's not a big change but has great benefit.

@RX14 RX14 added this to the Next milestone Oct 31, 2017
@RX14 RX14 merged commit 2e00b8f into crystal-lang:master Oct 31, 2017
@straight-shoota straight-shoota deleted the jm-json-docs branch November 1, 2017 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants