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

Constructors marked with @interface should not receive params #461

Closed
anandthakker opened this issue Jun 21, 2016 · 4 comments
Closed

Constructors marked with @interface should not receive params #461

anandthakker opened this issue Jun 21, 2016 · 4 comments
Labels

Comments

@anandthakker
Copy link
Contributor

Currently, if @interface is used on a constructor function, documentation still gives it a params list. IMO, the constructor params should be dropped in this case, since the actual constructor and its params are really implementation-specific, and so I think the @interface tag implies an intent not to document them.

@tmcw @jfirebaugh what do you think?

anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Jun 21, 2016
anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Jul 1, 2016
@tmcw
Copy link
Member

tmcw commented Jul 11, 2016

👍 I think so, but if you want to be sure, check with jsdoc to see if it respects @param tags along with interface.

anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Jul 14, 2016
@anandthakker
Copy link
Contributor Author

Hmm -- jsdoc doesn't say one way or the other.... and really, depending on your meaning, a constructor's signature really could be part of an interface, or it could be an implementation detail.

In the absence of clearer guidance from the spec, maybe a safeish path would be:

  • If @interface is used on a constructor, keep the params
  • If it's used in a "virtual" comment not tagged to a constructor, omit params unless that comment includes at least one @param or @returns
  • If @name is included, follow the previous rule as well

anandthakker pushed a commit to mapbox/mapbox-gl-js that referenced this issue Jul 18, 2016
lucaswoj pushed a commit to mapbox/mapbox-gl-js that referenced this issue Jul 20, 2016
lucaswoj pushed a commit to mapbox/mapbox-gl-js that referenced this issue Jul 20, 2016
* Major refactor to Source interface

 * Invert the TilePyramid / Source dependency
 * Change Source from class-based to factory-based interface
 * Add WorkerSource concept, allowing Source type to register worker-side functionality
 * Separate GeoJSONSource and ClusteredGeoJSONSource

* Add optional callback for dispatcher.broadcast

* Move coveringZoomLevel, coveringTiles to Transform

* Refactor raster source to new interface

* WIP - refactor video source

* Reload pyramid in onChange()

* Reinstate query*Features

* Restore validateStyle to top of style init

* Small edits from PR comments

* Go back to mixing in Evented in Source impls

* Drop Source.is

Removes the ability to create a `Source` instance independent of the
`Map` (or, actually, `Style` instance).

* Enforce correct source.id value

* s/cb/callback

* Fix extend/inherit and other minor errors

* Use more descriptive names for Source methods

* Rename TilePyramid to SourceCache

* Remove bucketStats

* Remove unused var

* Simplify GeoJSONWorkerSource

* Fix geojson_source.test

* Fix source_cache tests

* s/tile_pyramid/source_cache

* Restore 'ignore reload' test, now in source_cache

* Fix vector_tile_source.test

* Fix style.test

* Fix map tests

* Remove async loading of core source types

* s/query-features.js/query_features.js

* Replace old queryRenderedFeature edge case test

* Replace old GeoJSONSource test

Instead of checking whether GeoJSONSource#setData results in the TilePyramid dropping its loaded tiles, we now check whether SourceCache, on observing a `change` event from its underlying source, seeks to re-loadTile its tiles.

* Improve getSource API

* Treat errored source as loaded()

* Add some docs to geojson_worker_source

* Add supercluster-specific debug page

* Fix typo

* Fix 'geojson / reparse-overscaled' render test

* Fix lint

* Correctly handle Source error events

* WIP - add more docs

* Fix generated doc output

* Do not call prepare() unless source is ready

* Fix VideoSource

* Add image.html debug page

* Fix ImageSource

* Fix render tests

* Revise addSourceType API and add basic tests.

* Make GeoJSONSource more hackable

* Suppress signature and params in @interface docs

Related issue upstream: documentationjs/documentation#461

* Improve custom source type docs

* Update GeoJSONSource,VideoSource,ImageSource docs

* Throw from addSource if source.type is missing

* Update GeoJSON source examples

* load-tilejson => load_tilejson

* Move _sendDataToWorker to setData

* Revert to new-based API for sources

* Drop extra cluster stuff from default debug page.

Use cluster.html instead.

* Dedupe debug & test-suite assets

* Factor VectorTileWorkerSource out of Worker

* Fix tests

* Update registerWorkerSource to current WorkerSource interface

* Fix test after rebase

* Image/Video: tile.loaded|errored => tile.state

* Restore *.tile events on sources

* Hide custom source classes / interfaces from documentation

We should validate the custom source architecture and build some demo
sources before documenting the internals publicly.

* Namespace mapbox-gl-test-suite files in debug server

* Add source docs

* Remove GeoJSONSource, VideoSource, ImageSource from the public interface

* Minor GeoJSONSource simplification
@tmcw tmcw added the bug label Nov 23, 2016
@tmcw
Copy link
Member

tmcw commented Jul 28, 2017

Any chance we can wrangle a PR from those fixes upstream and get some good open source vibes going?

@tmcw
Copy link
Member

tmcw commented Jul 28, 2017

Merging into #675

@tmcw tmcw closed this as completed Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants