-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
JSON output for docs #4746
Conversation
2682f40
to
e87a71f
Compare
Any feedback on the |
There was a problem hiding this 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}}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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,
.
64ade24
to
13da880
Compare
13da880
to
6dbcead
Compare
6dbcead
to
50aea41
Compare
@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. |
Progress to get fuzzy doc search merged, probably the most anticipated feature for me. |
@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:
@RX14 Is fuzzy search here in this PR? |
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. |
Hhhhhhmmmmmmmm.... I thought this was giving a JSON output for docs when you passed The way I see it, we should have |
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. |
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.
TheItem.def_to_json
macros are essentially theto_json
part ofJSON.mapping
. I'd like to move them to theJSON
namespace because I think it is a common use case to define onlyto_json
methods when you neither want to parse JSON nor to automatically define instance variables asJSON.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