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

Improve the types generation and API documentation (PR 12102 follow-up) #12156

Merged
merged 4 commits into from
Aug 4, 2020

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Aug 2, 2020

The commit messages contain more information about the individual changes. I recommend looking at the individual commits with the ?w=1 flag since that reduces the diff size for some of them.

/cc @Snuffleupagus since this should fix the consistency problems and your last comments on the types patch.

@timvandermeij timvandermeij changed the title Improve API documentation Improve the API documentation Aug 2, 2020
@timvandermeij timvandermeij changed the title Improve the API documentation Improve the types generation and API documentation (PR 12102 follow-up) Aug 2, 2020
@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Aug 2, 2020

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/7b906377f212a83/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 2, 2020

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/7b906377f212a83/output.txt

Total script time: 3.46 mins

Published

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Looks much better already, however I couldn't help but spot a couple of (mostly pre-existing) issues.

Also, would it be possible to re-format this somehow, e.g. with line-breaks, to remove the eslint-disable/eslint-enable statements and improve overall readability (the ordering is also slightly weird, since Uint8ClampedArray should follow Uint8Array):

pdf.js/src/display/api.js

Lines 100 to 104 in 00a8b42

/* eslint-disable max-len */
/**
* @typedef {Int8Array | Uint8Array | Int16Array | Uint16Array | Int32Array | Uint32Array | Uint8ClampedArray | Float32Array | Float64Array} TypedArray
*/
/* eslint-enable max-len */

@timvandermeij timvandermeij force-pushed the types-followup branch 2 times, most recently from d253e39 to bd55f33 Compare August 3, 2020 22:20
@timvandermeij
Copy link
Contributor Author

For easier review, the interdiffs at https://github.com/mozilla/pdf.js/compare/3f38d3fd2b8a284b3d7cce2b4b97f03c4f958e94..d253e3923222f14a3cc5062cd83b7b018e3b4d67 (ignore the last four files since they are from a merge of the existing master branch) and https://github.com/mozilla/pdf.js/compare/d253e3923222f14a3cc5062cd83b7b018e3b4d67..bd55f3312d510f1a5ef2e34471b911797f9a0cc0 (for removing the ESLint ignores) display the changes I made.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2020

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/aefb8d821630503/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2020

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/aefb8d821630503/output.txt

Total script time: 3.49 mins

Published

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Much better, thank you for doing this!

This commit:
- moves the preparation work to a new `typestest-pre` target similar to
  how the other targets work;
- moves the `TYPESTEST_DIR` definition to the top of the file like we
  did for all other directory variables;
- renames the `TYPES_BUILD_DIR` variable to `TYPES_DIR` since it's
  shorter and the naming scheme then corresponds to the other directory
  variables;
- switches to `const`/template strings in the types targets where needed;
- converts the `if (err !== null)` check to `if (err)` similar to other
  targets.
This commit:
- formats the documentation block according to the standards;
- replaces the callback definitions with the `function` type (we have
  that for other definitions already and the callback type was not
  rendered correctly by JSDoc);
- synchronizes the type documentation and the class documentation;
- fixes the documentation by making it easier to read and making sure
  that all optional properties are indicated as such;
- uses the `@link` tag to indicate links to other code.

The `typestest` still passes and JSDoc now renders this class correctly.
Over time we used multiple different formats for JSDoc comments. This
commit standardizes those formats to the one we used most often.

Moreover, this removes the example in the outline endpoint documentation
since it now has a proper type definition and it didn't render correctly
in JSDoc.
@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2020

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/7c75e8fb3b58b22/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 4, 2020

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/7c75e8fb3b58b22/output.txt

Total script time: 3.51 mins

Published

@timvandermeij timvandermeij merged commit 71f8e1f into mozilla:master Aug 4, 2020
@timvandermeij timvandermeij deleted the types-followup branch August 4, 2020 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants