docs(env): Add JSDoc to Environment and API prototypes #1107
Conversation
ExE-Boss
force-pushed
the
docs/environment-prototypes
branch
2 times, most recently
from
8300d38
to
582f4d0
Compare
ExE-Boss
changed the title
| * New file urls include attachment host, so we don't need to | ||
| * prepend it | ||
| */ | ||
| var fUrl = url.parse(file_url); |
There was a problem hiding this comment.
Previously, url would’ve been a string due to it being shadowed by the file_url variable, which broke Embed_Text: #1047 (review)
| async function subpages(path, depth, self) { | ||
| var url = util.apiURL((path ? path : this.env.url) + '$children'); | ||
| // @ts-ignore: Unsafe `parseInt(number)` call | ||
| var depth_check = parseInt(depth); |
There was a problem hiding this comment.
This is unsafe because 0.0000001 stringifies to 1e-7, which results in parseInt(0.0000001) returning 1.
Same for 1000000000000000000000 (stringifies to 1e+21, so parseInt(…) returns 1).
See also: microsoft/TypeScript#15122
| async function subpagesExpand(path, depth, self) { | ||
| var url = util.apiURL((path ? path : this.env.url) + '$children?expand'); | ||
| // @ts-ignore: Unsafe `parseInt(number)` call | ||
| var depth_check = parseInt(depth); |
| * @deprecated Use `str.startsWith(sub_str)` | ||
| */ | ||
| function startsWith(str, sub_str) { | ||
| return String(str).indexOf(sub_str) === 0; |
There was a problem hiding this comment.
String(str) is faster than ('' + str), especially when str is already a string.
src/api/util.js
Outdated
| defaults, | ||
| promiseify, |
There was a problem hiding this comment.
util.promiseify isn’t used outside of util.js.
| @@ -188,15 +232,21 @@ async function tree(path, depth, self, reverse, ordered) { | |||
|
|
|||
| while ((i = (j = t.charAt(x++)).charCodeAt(0))) { | |||
| var m = i == 46 || (i >= 48 && i <= 57); | |||
| // @ts-ignore: This condition will always return 'true' since the types 'boolean' and 'number' have no overlap. | |||
| if (m !== n) { | |||
There was a problem hiding this comment.
See #1100 (review)
| if (m !== n) { | ||
| tz[++y] = ''; | ||
| // @ts-ignore: Type 'boolean' is not assignable to type 'number'. | ||
| n = m; |
There was a problem hiding this comment.
See #1100 (review)
| @@ -205,6 +255,8 @@ async function tree(path, depth, self, reverse, ordered) { | |||
| if (aa[x] !== bb[x]) { | |||
| var c = Number(aa[x]), | |||
| d = Number(bb[x]); | |||
|
|
|||
| // @ts-ignore: Loose comparison of number and string | |||
| if (c == aa[x] && d == bb[x]) { | |||
There was a problem hiding this comment.
| @@ -221,6 +277,8 @@ async function tree(path, depth, self, reverse, ordered) { | |||
| if (aa[x] !== bb[x]) { | |||
| var c = Number(aa[x]), | |||
| d = Number(bb[x]); | |||
|
|
|||
| // @ts-ignore: Loose comparison of number and string | |||
| if (c == aa[x] && d == bb[x]) { | |||
There was a problem hiding this comment.
| * TODO(djf): The *Prototype objects could each be defined in a | ||
| * separate src/api/*.js file and imported with require(). That would | ||
| * be tidier, and would keep separate the code with poor test coverage | ||
| * from the rest of the file which is well tested. |
There was a problem hiding this comment.
Because I hadn’t touched the JSDoc in #1107, I forgot to remove this TODO.
|
@ExE-Boss: the "commit of interest" you mention looks like it is only changes to comments, so that would be easy to approve. But that commit can only be merged if the one before it is merged, and that other commit is a huge change that would take hours of careful review to validate. Cleaning up this part of the codebase is simply not a priority to us at this point and I don't think it is likely that I or anyone with commit access to this repo will have time for this kind of review in 2019. |
|
Commit df96a50?w=1 doesn’t affect any code, this can be verified by disabling whitespace diff by adding the That’s why I did this across two commits. |
|
I never knew about the Okay, so my objection about this taking a long time to review is not valid, I guess. But I still don't get the point. Why are you doing this work? Why should I take time away from my prioritized work this sprint to review it? Who will benefit if we make these changes? These are sincere questions. This PR isn't associated with a bug, and you haven't include commit messages explaining the benefit of the changes. I understand, I think, that having typescript comments in the the files will make your tools work better. But to what end? Once you have typescript and intellisense set up in this codebase, what comes next? I feel like you must have some plan or vision for KumaScript that I am not aware of. I think that the prevailing feeling on the MDN team is that we'd like to retire it as soon as we can, that it is in maintenance mode, and not really worth any effort except for fixing bugs that actually impact the MDN site. As a general thing, it seems like it would be better to talk with us on IRC before starting big refactors like this to see if they align with team priorities and determine whether anyone will be available for reviews. |
|
review?(@davidflanagan): This is a blocker for bug 1519579 (#969). |
The commit of interest is 582f4d0.
The first refactoring commit needs to be reviewed with whitespace‑only changes hidden: df96a50?w=1
review?(@davidflanagan, @wbamberg)