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

Eslint JSDoc fixes #8960

Merged
merged 11 commits into from
Mar 2, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/data/array_types.js
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,7 @@ export class CollisionBoxArray extends StructArrayLayout6i1ul2ui2i24 {
/**
* Return the CollisionBoxStruct at the given location in the array.
* @param {number} index The index of the element.
* @private
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to prevent redundantly specifying @private on every method if the parent class is marked @private anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is not supported. From the JSDoc docs:

The @Private tag is not inherited by child members. For example, if the @Private tag is added to a namespace, members of the namespace can still appear in the generated output; because the namespace is private, the members' namepath will not include the namespace.

*/
get(index: number): CollisionBoxStruct {
assert(!this.isTransferred);
Expand Down Expand Up @@ -934,6 +935,7 @@ export class PlacedSymbolArray extends StructArrayLayout2i2ui3ul3ui2f3ub1ul1i48
/**
* Return the PlacedSymbolStruct at the given location in the array.
* @param {number} index The index of the element.
* @private
*/
get(index: number): PlacedSymbolStruct {
assert(!this.isTransferred);
Expand Down Expand Up @@ -1036,6 +1038,7 @@ export class SymbolInstanceArray extends StructArrayLayout8i14ui1ul3f60 {
/**
* Return the SymbolInstanceStruct at the given location in the array.
* @param {number} index The index of the element.
* @private
*/
get(index: number): SymbolInstanceStruct {
assert(!this.isTransferred);
Expand Down Expand Up @@ -1064,6 +1067,7 @@ export class GlyphOffsetArray extends StructArrayLayout1f4 {
/**
* Return the GlyphOffsetStruct at the given location in the array.
* @param {number} index The index of the element.
* @private
*/
get(index: number): GlyphOffsetStruct {
assert(!this.isTransferred);
Expand Down Expand Up @@ -1100,6 +1104,7 @@ export class SymbolLineVertexArray extends StructArrayLayout3i6 {
/**
* Return the SymbolLineVertexStruct at the given location in the array.
* @param {number} index The index of the element.
* @private
*/
get(index: number): SymbolLineVertexStruct {
assert(!this.isTransferred);
Expand Down Expand Up @@ -1133,6 +1138,7 @@ export class FeatureIndexArray extends StructArrayLayout1ul2ui8 {
/**
* Return the FeatureIndexStruct at the given location in the array.
* @param {number} index The index of the element.
* @private
*/
get(index: number): FeatureIndexStruct {
assert(!this.isTransferred);
Expand Down
4 changes: 2 additions & 2 deletions src/geo/lng_lat_bounds.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class LngLatBounds {
/**
* Set the northeast corner of the bounding box
*
* @param {LngLatLike} ne
* @param {LngLatLike} ne a {@link LngLatLike} object describing the northeast corner of the bounding box.
* @returns {LngLatBounds} `this`
*/
setNorthEast(ne: LngLatLike) {
Expand All @@ -52,7 +52,7 @@ class LngLatBounds {
/**
* Set the southwest corner of the bounding box
*
* @param {LngLatLike} sw
* @param {LngLatLike} sw a {@link LngLatLike} object describing the southwest corner of the bounding box.
* @returns {LngLatBounds} `this`
*/
setSouthWest(sw: LngLatLike) {
Expand Down
18 changes: 14 additions & 4 deletions src/geo/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,10 @@ class Transform {

/**
* Return a zoom level that will cover all tiles the transform
* @param {Object} options
* @param {number} options.tileSize
* @param {boolean} options.roundZoom
* @returns {number} zoom level
* @param {Object} options options
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of options params. It might not be worthwhile to add descriptions to each, but I am not sure how to configure eslint to ignore them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see how this looks on the live docs — we might need to come up with a way to filter this out or maybe PR the plugin repo with an option to ignore.

* @param {number} options.tileSize Tile size, expressed in pixels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Might be worth mentioning what pixel space, in this case logical pixels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karimnaaji I'm don't think our end users will understand what we mean by "logical pixels." How about "screen pixels" instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work I think, I've seen Mapkit make use of CSS pixels to denote the same, anything that suggests invariance over pixel resolution/retina displays is good.

* @param {boolean} options.roundZoom Target zoom level. This value will be rounded to the nearest integer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to express what happens in both cases here, what about the following?

If true, the value will be rounded to the closest integer, otherwise the value will be floored.

* @returns {number} zoom level An integer zoom level at which all tiles will be visible.
*/
coveringZoomLevel(options: {roundZoom?: boolean, tileSize: number}) {
const z = (options.roundZoom ? Math.round : Math.floor)(
Expand Down Expand Up @@ -259,6 +259,7 @@ class Transform {
* @param {boolean} options.reparseOverscaled
* @param {boolean} options.renderWorldCopies
* @returns {Array<OverscaledTileID>} OverscaledTileIDs
* @private
*/
coveringTiles(
options: {
Expand Down Expand Up @@ -408,6 +409,7 @@ class Transform {
* Given a location, return the screen point that corresponds to it
* @param {LngLat} lnglat location
* @returns {Point} screen point
* @private
*/
locationPoint(lnglat: LngLat) {
return this.coordinatePoint(this.locationCoordinate(lnglat));
Expand All @@ -417,6 +419,7 @@ class Transform {
* Given a point on screen, return its lnglat
* @param {Point} p screen point
* @returns {LngLat} lnglat location
* @private
*/
pointLocation(p: Point) {
return this.coordinateLocation(this.pointCoordinate(p));
Expand All @@ -427,6 +430,7 @@ class Transform {
* coordinate that represents it at this transform's zoom level.
* @param {LngLat} lnglat
* @returns {Coordinate}
* @private
*/
locationCoordinate(lnglat: LngLat) {
return MercatorCoordinate.fromLngLat(lnglat);
Expand All @@ -436,6 +440,7 @@ class Transform {
* Given a Coordinate, return its geographical position.
* @param {Coordinate} coord
* @returns {LngLat} lnglat
* @private
*/
coordinateLocation(coord: MercatorCoordinate) {
return coord.toLngLat();
Expand Down Expand Up @@ -473,6 +478,7 @@ class Transform {
* Given a coordinate, return the screen point that corresponds to it
* @param {Coordinate} coord
* @returns {Point} screen point
* @private
*/
coordinatePoint(coord: MercatorCoordinate) {
const p = [coord.x * this.worldSize, coord.y * this.worldSize, 0, 1];
Expand All @@ -483,6 +489,7 @@ class Transform {
/**
* Returns the map's geographical bounds. When the bearing or pitch is non-zero, the visible region is not
* an axis-aligned rectangle, and the result is the smallest bounds that encompasses the visible region.
* @returns {LngLatBounds} Returns a {@link LngLatBounds} object describing the map's geographical bounds.
*/
getBounds(): LngLatBounds {
return new LngLatBounds()
Expand All @@ -494,6 +501,7 @@ class Transform {

/**
* Returns the maximum geographical bounds the map is constrained to, or `null` if none set.
* @returns {LngLatBounds} {@link LngLatBounds}
*/
getMaxBounds(): LngLatBounds | null {
if (!this.latRange || this.latRange.length !== 2 ||
Expand All @@ -504,6 +512,7 @@ class Transform {

/**
* Sets or clears the map's geographical constraints.
* @param {LngLatBounds} bounds A {@link LngLatBounds} object describing the new geographic boundaries of the map.
*/
setMaxBounds(bounds?: LngLatBounds) {
if (bounds) {
Expand All @@ -519,6 +528,7 @@ class Transform {
/**
* Calculate the posMatrix that, given a tile coordinate, would be used to display the tile on a map.
* @param {UnwrappedTileID} unwrappedTileID;
* @private
*/
calculatePosMatrix(unwrappedTileID: UnwrappedTileID, aligned: boolean = false): Float32Array {
const posMatrixKey = unwrappedTileID.key;
Expand Down
1 change: 1 addition & 0 deletions src/gl/vertex_buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class VertexBuffer {

/**
* @param dynamicDraw Whether this buffer will be repeatedly updated.
* @private
*/
constructor(context: Context, array: StructArray, attributes: $ReadOnlyArray<StructArrayMember>, dynamicDraw?: boolean) {
this.length = array.length;
Expand Down
4 changes: 4 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const exported = {
* Gets and sets the map's [access token](https://www.mapbox.com/help/define-access-token/).
*
* @var {string} accessToken
* @returns {string} The currently set access token.
* @example
* mapboxgl.accessToken = myAccessToken;
* @see [Display a map](https://www.mapbox.com/mapbox-gl-js/examples/)
Expand All @@ -67,6 +68,7 @@ const exported = {
* Gets and sets the map's default API URL for requesting tiles, styles, sprites, and glyphs
*
* @var {string} baseApiUrl
* @returns {string} The current base API URL.
* @example
* mapboxgl.baseApiUrl = 'https://api.mapbox.com';
*/
Expand All @@ -84,6 +86,7 @@ const exported = {
* Make sure to set this property before creating any map instances for it to have effect.
*
* @var {string} workerCount
* @returns {number} Number of workers currently configured.
* @example
* mapboxgl.workerCount = 2;
*/
Expand All @@ -100,6 +103,7 @@ const exported = {
* which affects performance in raster-heavy maps. 16 by default.
*
* @var {string} maxParallelImageRequests
* @returns {number} Number of parallel requests currently configured.
* @example
* mapboxgl.maxParallelImageRequests = 10;
*/
Expand Down
2 changes: 2 additions & 0 deletions src/render/painter.js
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ class Painter {
* Transform a matrix to incorporate the *-translate and *-translate-anchor properties into it.
* @param inViewportPixelUnitsUnits True when the units accepted by the matrix are in viewport pixels instead of tile units.
* @returns {Float32Array} matrix
* @private
*/
translatePosMatrix(matrix: Float32Array, tile: Tile, translate: [number, number], translateAnchor: 'map' | 'viewport', inViewportPixelUnitsUnits?: boolean) {
if (!translate[0] && !translate[1]) return matrix;
Expand Down Expand Up @@ -591,6 +592,7 @@ class Painter {
* Checks whether a pattern image is needed, and if it is, whether it is not loaded.
*
* @returns true if a needed image is missing and rendering needs to be skipped.
* @private
*/
isPatternMissing(image: ?CrossFaded<ResolvedImage>): boolean {
if (!image) return false;
Expand Down
4 changes: 0 additions & 4 deletions src/source/canvas_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,6 @@ class CanvasSource extends ImageSource {
* @method setCoordinates
* @instance
* @memberof CanvasSource
* @param {Array<Array<number>>} coordinates Four geographical coordinates,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to specify params for inherited methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this information expressed anywhere else after we delete it?

* represented as arrays of longitude and latitude numbers, which define the corners of the canvas.
* The coordinates start at the top left corner of the canvas and proceed in clockwise order.
* They do not have to represent a rectangle.
* @returns {CanvasSource} this
*/
// setCoordinates inherited from ImageSource
Expand Down
4 changes: 4 additions & 0 deletions src/source/geojson_worker_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
* @param [loadGeoJSON] Optional method for custom loading/parsing of
* GeoJSON based on parameters passed from the main-thread Source.
* See {@link GeoJSONWorkerSource#loadGeoJSON}.
* @private
*/
constructor(actor: Actor, layerIndex: StyleLayerIndex, availableImages: Array<string>, loadGeoJSON: ?LoadGeoJSON) {
super(actor, layerIndex, availableImages, loadGeoJSONTile);
Expand All @@ -126,6 +127,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
*
* @param params
* @param callback
* @private
*/
loadData(params: LoadGeoJSONParameters, callback: Callback<{
resourceTiming?: {[string]: Array<PerformanceResourceTiming>},
Expand Down Expand Up @@ -233,6 +235,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
*
* @param params
* @param params.uid The UID for this tile.
* @private
*/
reloadTile(params: WorkerTileParameters, callback: WorkerTileCallback) {
const loaded = this.loaded,
Expand All @@ -255,6 +258,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource {
* @param params
* @param [params.url] A URL to the remote GeoJSON data.
* @param [params.data] Literal GeoJSON data. Must be provided if `params.url` is not.
* @private
*/
loadGeoJSON(params: LoadGeoJSONParameters, callback: ResponseCallback<Object>) {
// Because of same origin issues, urls must either include an explicit
Expand Down
2 changes: 1 addition & 1 deletion src/source/image_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class ImageSource extends Evented implements Source {
* Updates the image URL and, optionally, the coordinates. To avoid having the image flash after changing,
* set the `raster-fade-duration` paint property on the raster layer to 0.
*
* @param {Object} options
* @param {Object} options Options object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the type annotations, should we use that instead of Object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good long-term goal, but I think it's out of scope for this change. Or is there a technique you know of to link type annotations automatically?

* @param {string} [options.url] Required image URL.
* @param {Array<Array<number>>} [options.coordinates] Four geographical coordinates,
* represented as arrays of longitude and latitude numbers, which define the corners of the image.
Expand Down
11 changes: 11 additions & 0 deletions src/source/source_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class SourceCache extends Evented {
/**
* Return true if no tile data is pending, tiles will not change unless
* an additional API call is received.
* @private
*/
loaded(): boolean {
if (this._sourceErrored) { return true; }
Expand Down Expand Up @@ -180,6 +181,7 @@ class SourceCache extends Evented {

/**
* Return all tile ids ordered with z-order, and cast to numbers
* @private
*/
getIds(): Array<string> {
return (values(this._tiles): any).map((tile: Tile) => tile.tileID).sort(compareTileId).map(id => id.key);
Expand Down Expand Up @@ -307,13 +309,15 @@ class SourceCache extends Evented {
}
/**
* Get a specific tile by TileID
* @private
*/
getTile(tileID: OverscaledTileID): Tile {
return this.getTileByID(tileID.key);
}

/**
* Get a specific tile by id
* @private
*/
getTileByID(id: string): Tile {
return this._tiles[id];
Expand All @@ -322,6 +326,7 @@ class SourceCache extends Evented {
/**
* For a given set of tiles, retain children that are loaded and have a zoom
* between `zoom` (exclusive) and `maxCoveringZoom` (inclusive)
* @private
*/
_retainLoadedChildren(
idealTiles: {[any]: OverscaledTileID},
Expand Down Expand Up @@ -367,6 +372,7 @@ class SourceCache extends Evented {

/**
* Find a loaded parent of the given tile (up to minCoveringZoom)
* @private
*/
findLoadedParent(tileID: OverscaledTileID, minCoveringZoom: number): ?Tile {
if (tileID.key in this._loadedParentTiles) {
Expand Down Expand Up @@ -403,6 +409,7 @@ class SourceCache extends Evented {
* Larger viewports use more tiles and need larger caches. Larger viewports
* are more likely to be found on devices with more memory and on pages where
* the map is more important.
* @private
*/
updateCacheSize(transform: Transform) {
const widthInTiles = Math.ceil(transform.width / this._source.tileSize) + 1;
Expand Down Expand Up @@ -462,6 +469,7 @@ class SourceCache extends Evented {
/**
* Removes tiles that are outside the viewport and adds new tiles that
* are inside the viewport.
* @private
*/
update(transform: Transform) {
this.transform = transform;
Expand Down Expand Up @@ -783,6 +791,7 @@ class SourceCache extends Evented {
* cover the given bounds.
* @param pointQueryGeometry coordinates of the corners of bounding rectangle
* @returns {Array<Object>} result items have {tile, minX, maxX, minY, maxY}, where min/max bounding values are the given bounds transformed in into the coordinate space of this tile.
* @private
*/
tilesIn(pointQueryGeometry: Array<Point>, maxPitchScaleFactor: number, has3DLayer: boolean) {

Expand Down Expand Up @@ -901,6 +910,7 @@ class SourceCache extends Evented {
/**
* Sets the set of keys that the tile depends on. This allows tiles to
* be reloaded when their dependencies change.
* @private
*/
setDependencies(tileKey: string, namespace: string, dependencies: Array<string>) {
const tile = this._tiles[tileKey];
Expand All @@ -911,6 +921,7 @@ class SourceCache extends Evented {

/**
* Reloads all tiles that depend on the given keys.
* @private
*/
reloadTilesForDependencies(namespaces: Array<string>, keys: Array<string>) {
for (const id in this._tiles) {
Expand Down
1 change: 1 addition & 0 deletions src/source/tile.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ class Tile {
/**
* @param {OverscaledTileID} tileID
* @param size
* @private
*/
constructor(tileID: OverscaledTileID, size: number) {
this.tileID = tileID;
Expand Down
2 changes: 2 additions & 0 deletions src/source/tile_cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,8 @@ class TileCache {
/**
* Remove entries that do not pass a filter function. Used for removing
* stale tiles from the cache.
*
* @param filterFn filter function to check.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure how to describe this param.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about describing what the function signature would be? Worthwhile information in that case would be what it should return and in each case how it would impact the filtering.

It's taking as input a Tile, and the function filterFn controls whether the tile would be filtered out during the filter process. What about:

@param filterFn filter function, takes as input a Tile, and controls whether the tile would be filtered out. If the function returns false, the tile would be filtered out, it won't otherwise.

Feel free to make it more succinct.

*/
filter(filterFn: (tile: Tile) => boolean) {
const removed = [];
Expand Down
Loading