Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2233 closed change (fixed)

Adapt array type annotations for JsDoc 3

Reported by: sebastian Assignee: sebastian
Priority: P3 Milestone: Adblock-Plus-2.6.10-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: trev, greiner Blocked By:
Blocking: #2231 Platform: Firefox
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5964611362750464
http://codereview.adblockplus.org/4911914341629952

Description

Background

Currently, we annotate arrays like @type Array of String. This syntax isn't supported anymore by JSDoc 3, and needs to be written as string[] instead.

What to change

Adapt array annotations for compatibility with JsDoc 3.

Change History (15)

comment:1 Changed 5 years ago by sebastian

  • Summary changed from Adapt array type annotations for JSDoc 3 to Adapt array type annotations for JsDoc 3

comment:2 Changed 5 years ago by trev

  • Platform changed from Unknown to Firefox/Firefox Mobile
  • Priority changed from Unknown to P3
  • Ready set

comment:3 Changed 5 years ago by greiner

What about usingArray<string> instead? Personally, I don't mind either way but it seems more consistent with Object<string, number>, for instance. Generally, there are quite a few variations possible in JSDoc (i.e. synonyms) so might make sense to agree on what we should or should not use.

Note that while JSDoc defines it as Array.<string> it also says that it supports Closure Compiler's syntax.

comment:4 Changed 5 years ago by greiner

  • Cc greiner added

comment:5 follow-up: Changed 5 years ago by sebastian

It's either Array.<string> or string[]. I'd prefer the latter, already used it a couple of times in the platform code. @trev: What do you prefer?

comment:6 Changed 5 years ago by trev

I definitely prefer the latter as well, it's more JavaScript-like. For objects we have no other way but to use this confusing syntax seemingly inspired by Java generics, hopefully it won't be necessary all too often.

comment:7 in reply to: ↑ 5 Changed 5 years ago by greiner

Replying to sebastian:

It's either Array.<string> or string[].

As I said, they state that they also support Closure Compiler's syntax which is Array<string> (see developers.google.com).

While the former is more Java-style, the latter seems more along the lines of C. So I wouldn't consider any of those to feel native to JavaScript which is why I'm arguing about consistency.

comment:8 Changed 5 years ago by sebastian

  • Owner set to sebastian

comment:9 follow-up: Changed 5 years ago by trev

With the same argument you could say that the concept of types is generally more along the lines of C and we should stop documenting types ;-)

While annotating types is generally foreign to JavaScript, string[] denoting an array should be obvious to any JavaScript developer. Similarly, {a: string, b: number} is obviously an object descriptor. Merely maps cannot be described with proper JavaScript-like syntax.

comment:10 in reply to: ↑ 9 Changed 5 years ago by greiner

Replying to trev:

With the same argument you could say that the concept of types is generally more along the lines of C and we should stop documenting types ;-)

My argument was more along the lines of char foo[];, where foo[] is not something you'd ever encounter in JS. Generally, JS's syntax is quite similar to C's so yes, people would be more familiar with it than the Java-style syntax because there are no < and > characters. ;)

While annotating types is generally foreign to JS, string[] denoting an array should be obvious to any JavaScript developer. Similarly, {a: string, b: number} is obviously an object descriptor. Merely maps cannot be described with proper JavaScript-like syntax.

An array is also only an Object so why not treat it as such as well? The issue I see is that *[] can only be used for pure arrays (not even for an object that extends Array as far as I understand) so it appears quite limited when compared to Array<*>.

PS: I had to use "JS" to circumvent our spam filter, not because I'm lazy. :)

comment:11 Changed 5 years ago by sebastian

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:12 Changed 5 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:13 Changed 5 years ago by sebastian

  • Review URL(s) modified (diff)

comment:15 Changed 5 years ago by philll

  • Platform changed from Firefox/Firefox Mobile to Firefox

Made Firefox and Firefox mobile available as seperate platforms.

Note: See TracTickets for help on using tickets.