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

Do not include oli.* symbols in the API documentation #2072

Merged
merged 6 commits into from
May 23, 2014

Conversation

ahocevar
Copy link
Member

Instead, document exported symbols in their ol.* counterparts. This is a possible solution to an issue discussed in #2068 (comment).

With this change, all oli.* documentation goes away, except for oli.FrameState, which is not documented elsewhere.

@ahocevar
Copy link
Member Author

@elemoine Can you please have a look at ab10e05 to make sure that it does not break anything for you? Thanks.

@ahocevar
Copy link
Member Author

Looking at ab10e05 again, I think we can even move oli.FrameState to olx.FrameState, and oli.View2DState to olx.View2DState. New commit to come.

@elemoine
Copy link
Member

Yes, I'm interested to have a look at this.

@ahocevar
Copy link
Member Author

@elemoine I made the changes mentioned above. The new commit to look at is f203da7.

@elemoine
Copy link
Member

Ok I'll have a look when I can. I may have questions rather than pertinent comments :)

@ahocevar
Copy link
Member Author

@elemoine Just a reminder in case you forgot about this.

@elemoine
Copy link
Member

Thanks for the reminder @ahocevar.

I think it's ok. But I also think we should treat the event types (e.g. ol.CollectionEvent) and ol.FrameState (and ol.View2DState) in a similar way. This means we should define an ol.FrameState constructor in the library that inherits from an oli.FrameState interface defined in the oli.js externs. In olx.js we define types for options objects created by application/external code, and nothing else. Things that the library creates itself and that shouldn't be renamed are defined and documented in the library code, and interfaces are defined in oli.js to prevent renaming. I hope it makes sense.

@ahocevar
Copy link
Member Author

Not sure I understand. The different between the events (e.g. ol.CollectionEvent) and ol.FrameState/ol.View2DState is that the former are classes, whereas the latter are object literals. Changing ol.FrameState and ol.View2DState from object literals to classes would add unnecessary overhead I think.

@elemoine
Copy link
Member

Not sure I understand. The different between the events (e.g. ol.CollectionEvent) and ol.FrameState/ol.View2DState is that the former are classes, whereas the latter are object literals.

I was suggesting to make ol.FrameState and ol.View2DState constructors for consistency with the event types. I do not see why they'd be treated differently.

Changing ol.FrameState and ol.View2DState from object literals to classes would add unnecessary overhead I think.

Is doing var fs = new ol.FrameState(); f.extent = extent; … more expensive than doing var fs = {extent: extent, …}. I don't know.

@elemoine
Copy link
Member

No need to change this now. I'm fine with moving ol.FrameState and ol.View2DState to olx.js (maybe in a clearly identified section).

@ahocevar
Copy link
Member Author

Thanks @elemoine. I updated externs/olx.js to have all library defined object literals in their own section.

@elemoine
Copy link
Member

Cool. Thanks. Please merge.

@ahocevar
Copy link
Member Author

One more thing. As discussed with @tschaub, I also moved the @todo api annotations from externs/oli.js to the class definitions in the ol namespace. This saves us from having to add too much code to JSDoc plugins, and gets rid of the separation of documentation (in the class) and @todo api annotation (in the interface).

Instead, make sure that the properties are documented in the
implementing class.
@ahocevar
Copy link
Member Author

I think I addressed everything now, also what was discussed in the hangout today.

@elemoine
Copy link
Member

Looks good to me.

ahocevar added a commit that referenced this pull request May 23, 2014
Do not include oli.* symbols in the API documentation
@ahocevar ahocevar merged commit 0e7f86e into openlayers:master May 23, 2014
@ahocevar ahocevar deleted the no-oli-docs branch May 23, 2014 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants