google / stijl

Unified code review dashboard.
Apache License 2.0
41 stars 13 forks source link

Extract Redux state type schema into a file. #18

Closed hidehikoabe closed 7 years ago

hidehikoabe commented 8 years ago

The schame is useful to document. The goal is to make the documentation easier in the following changes.

hidehikoabe commented 8 years ago

Note: we may want to use the name "state" for variables, and I'm slightly worried about the conflict. Welcome better suggestions :-).

nya3jp commented 8 years ago

I really appreciate your effort to add more documentations!

But in this case, it seems to me that converting EXAMPLE_STATE into a bunch of jsdocs is not super improvement... For me, EXAMPLE_STATE is easier to read because I can learn the whole structure of a redux state is like at a glance, while jsdoc is sparse and a bit harder to read.

Of course the story will be very different if we run type checks with those jsdocs, but I don't have any concrete plan to enable it yet.

I guess you may want to continue adding more documentation / type information to the code, let's decide the goal before continuing the work. If we want to enable some automated type checking, we should plan how to enable it before adding more type information (e.g. jsdoc or typescript). Otherwise, I think we don't need to stick to jsdocs; any documentation easier to read by humans is the best. WDYT?

hidehikoabe commented 8 years ago

Thank you for review and comments. Before updating the code, let's decide a way to go. Here is what I was thinking;

What I'd like to do in this is following three items;

I believe you'll agree on the first item. So, the main topic is the second and third item, which are for mainly documentation.

I prefer annotating types of function params/return value somehow in simpler form, which will help me to understand how those functions should be. Practically, I'm giving it a try to use JsDoc, but I'm also fine in other way (incl. EXAMPLE_STATE style as you said) if it is clear enough. However, current EXAMPLE_STATE style does not have names for types, so it is difficult to be referred from other places, like function documents. Also, for me, during the code reading, it was difficult to understand what fields an object should have, and what values are expected for those fields. Those are what I'd like to resolve in this request, and the goal of the second and third items.

As for auto type-checking (or using TypeScript), IMHO it sounds nice-to-have, but also sounds overkill for now, considering our limited resources, and the size of the project. So I think it is ok to make it as out-of-focus.

An alternative (random) proposal is using @type. E.g.; EXAMPLESTATE = { /\ @type {Config} / config: { /* @type {Site[]} / sites: [ { /* @type {string} / label: 'Label of the code review site', /* @type {string} / url: 'URL of the code review site top page', /_* @type {SiteType} */ type: 'gerrit/rietveld', }, ] }

though, it looks not-formal JsDoc usage. Or, if you still think the annotation is ugly, I'm happy to hear your better approach.

nya3jp commented 8 years ago

Your suggestion to add @type to EXAMPLE_STATE sounds good to me. How about trying it?

hidehikoabe commented 8 years ago

Updated. PTAL.

hidehikoabe commented 8 years ago

Note: I renamed state.js to states.js (singular to plural), just for avoiding conflict.

nya3jp commented 8 years ago

lgtm with nits. Thanks!

nya3jp commented 7 years ago

I will merge this change now and fix nits by myself.