ibm-js / jsdoc-amddcl

An effort to create a JSDoc template that works well with AMD and C3MRO-based multiple class inheritance.
Other
3 stars 9 forks source link

@augments supporting mixin. #35

Closed asudoh closed 10 years ago

asudoh commented 10 years ago

I think this addresses one of the issues @sbrunot hit. CC @AdrianVasiliu

AdrianVasiliu commented 10 years ago

I'm not sure I'm getting it correctly. Before this change, we were already getting the expected documentation (as far as I know) for classes marked with @mixin plus @augments (such as https://github.com/ibm-js/jsdoc-amddcl/blob/master/sampleprojects/sampleproject/MixinInheritingImported.js or https://github.com/ibm-js/delite/blob/master/Scrollable.js). Sebastien's issue (one of them) was with inner classes (not modules) marked @private plus @mixin, for which @lends has been found to hurt. Do I miss something?

asudoh commented 10 years ago

If I understand some of the conversations between you and @sbrunot correctly; If you take a look at http://yourserver/path/to/jsdoc-amddcl/out/sampleproject/0.1.0-dev/MixinInheritingInHouse.html, you’ll find methodOfBaseCommon(), methodOfBase0() are methodOfBase() are not there even though sampleproject/MixinInheritingInHouse inherits sampleproject/Base0 and sampleproject/Base1. It may be a correct behaviour from JSDoc perspective, though it’s not in line with our notion of “mixin”. @sbrunot told us that members from delite/Store not appearing in delite/list/List, which is caused by such JSDoc’s behaviour with mixin. Wondering if it makes sense to you.

AdrianVasiliu commented 10 years ago

@sbrunot told us that members from delite/Store not appearing in delite/list/List

My understanding of the complaint is different. List wants to customize the doc of the properties store and query that it inherits from delite/Store. For this purpose, List has JSDoc comments without code: https://github.com/ibm-js/deliteful/blob/master/list/List.js#L59 . Now, despite these comments, these members were not showing up in the doc. For testing purposes, I tried to remove all other members of List, and the problem went away, which indicated that the issue would be due to some side-effect of (wrong) JSDoc comments on the other members. I'm not sure whether Sebastien made progress on it, but for me this issue was about pseudo-members not showing up.

I think members of superclasses not showing up directly in the doc of subclasses is, as you say, correct from a JSDoc perspective. Moreover, I think we would should keep this behavior. I don't see a good reason to make an exception for members inherited from mixins. For me, the same reasoning applies for them as for members inherited from any other parent class: if a subclass would show the inherited members, with our deep inheritance we get often tons of APIs which do not allow to focus on the API's specifically added by the given class itself. Given that we include links to the doc of the parent classes (including mixins), this should be just fine.

asudoh commented 10 years ago

Seems that I misread what @sburnot is stumbled at (sorry for this). I quickly run with latest jsdoc-amddcl and I see .query and .store show up in the doc.

Let me be more clear on the inheritance topic. Current JSDoc code (w/o my change) makes inherited members show up in the doc only for class (not for mixin). Wondering if it puts us on the same page here.

AdrianVasiliu commented 10 years ago

Might be my fault, but no, it isn't clearer to me. So you say that, for a class A which is not tagged with @mixin and inherits from class B (which itself is a mixin? or it is not? or it does not matter?), the doc of A used to show the members of B before your change today, and after your change this holds even if A is a mixin? If this is what you mean, it surprises me. I never noticed members from a superclass showing up in the subclass (except of course for overrides and pseudo-members). And, as said above, if this would happen, I wouldn't think it's a good thing.

asudoh commented 10 years ago

@AdrianVasiliu You are correct. I thought you are aware of it, though I may have been wrong.

AdrianVasiliu commented 10 years ago

Oh. Thanks for reminding me about it! I meant that I didn't notice inherited members showing up nowadays with our delite/deliteful code, but checking it now it's exactly as you say, it depends on the mixin tag. Anyway, happy that the misunderstanding is gone - from the mentioning of Sebastien's complaint about "members from delite/Store not appearing in delite/list/List", I thought your change makes appear the inherited members; but in fact it's exactly the contrary: it hides them in a case when they used to be shown. Good!

asudoh commented 10 years ago

Oh… I made inherited properties appear (to avoid data loss). I see this is not what you wanted. I’ll see how https://github.com/ibm-js/jsdoc-amddcl/issues/10 can be addressed.

AdrianVasiliu commented 10 years ago

Oh, so this is an iterative process for putting us on the same page ;-) I hope I'm not wrong (again) that this time we really removed the misunderstanding.

I’ll see how #10 can be addressed.

Thanks, but to clarify, in my eyes #10 (that is the addition of a GUI element allowing the reader to turn on/off the inherited properties) is a "nice to have", not high priority. And, regardless of #10 being implemented or not, I think inherited members shouldn't show up by default (with or without mixins).

asudoh commented 10 years ago

OK fixed with: https://github.com/ibm-js/jsdoc-amddcl/commit/b1395be36b3d43d0bcc2579348cd4e961068b235

AdrianVasiliu commented 10 years ago

Wow, great!