Opened 12 months ago

Closed 3 months ago

#7103 closed change (rejected)

Add docs npm script to adblockpluschrome

Reported by: erikvold Assignee:
Priority: Unknown Milestone: Adblock-Plus-3.4.3-for-Chrome-Opera-Firefox
Module: Platform Keywords: closed-in-favor-of-gitlab
Cc: jsonesen, tlucas, sebastian, greiner, wspee, mjethani, kzar Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29937555
https://codereview.adblockplus.org/29984609
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/16

Description (last modified by jsonesen)

As part of the effort to reduce the dependency on buildtools it would be useful to convert the docs to be generated via npm by configuring package.json accordingly resulting in a user level change from ./build.py docs docs to npm run docs.

Changes

add a script called "docs" to package.json and the jsdoc dependency.

Note

Doing this would be most simple if we have the output of jsdoc go into "docs" in the root directory of the repository.

Change History (34)

comment:1 Changed 12 months ago by jsonesen

  • Cc tlucas sebastian greiner wspee mjethani added
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 12 months ago by jsonesen

  • Cc kzar added

comment:3 Changed 12 months ago by jsonesen

Manish, Winsley, Thomas,

Is this something you all are okay with? If so I have patch sets prepared. This will be useful for us to not depend on buildtools to generate docs.

comment:4 Changed 12 months ago by erikvold

  • Description modified (diff)

comment:5 Changed 12 months ago by sebastian

For reference, this is part of a larger effort to phase out (the Python-based) buildtools. The changes itself look good to me, the next step would be integrating this with GitLab CI/CD.

Last edited 12 months ago by sebastian (previous) (diff)

comment:6 Changed 12 months ago by mjethani

Makes sense to me.

comment:7 follow-up: Changed 10 months ago by jsonesen

I just noticed that abpui is pretty baren from a docstring standpoint, should it still be added there? Definitely makes sense for abpchrome and abpcore

comment:8 Changed 10 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:9 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 7103 - Add jsdoc to package.json

comment:10 Changed 10 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:11 Changed 10 months ago by jsonesen

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Type changed from defect to change

comment:12 in reply to: ↑ 7 Changed 10 months ago by greiner

Replying to jsonesen:

I just noticed that abpui is pretty baren from a docstring standpoint, should it still be added there? Definitely makes sense for abpchrome and abpcore

Feel free to create a ticket for it, if you think it'd be useful.

comment:13 Changed 10 months ago by sebastian

  • Summary changed from Add docs npm script to adblockpluscore, adblockpluschrome, and adblockplusui to Add docs npm script to adblockpluscore and adblockpluschrome

This issue is about getting rid of the buildtools dependency for generating the docs. We currently don't provide any API documentation for adblockplusui (not through buildtools anyway).

Whether to provide API documentation for adblockplusui in the future is unrelated of this issue.
FWIW, I don't think that this would be too useful anyway, as (unlike for adblockpluscore and adblockpluschrome) there are no consumers on the API-level of the user interface.

comment:14 Changed 10 months ago by mjethani

Just want to add that it would be really nice if after every commit the latest version of the documentation was available on some website. I don't know if this has already been set up. Anyway, it's unrelated to the issue, but I just wanted to mention it.

comment:15 Changed 10 months ago by sebastian

We have that already. However, currently sitescripts is calling build.py docs. This issue is about doing that through an npm script instead, and eventually running it from GitLab CI.

comment:16 Changed 10 months ago by tlucas

FWIW, we might be able to use gitlab-pages for that purpose, after we landed this.

comment:17 Changed 10 months ago by sebastian

  • Milestone changed from Adblock-Plus-for-Chrome-Opera-Firefox-next to Adblock-Plus-3.4.3-for-Chrome-Opera-Firefox

comment:18 Changed 10 months ago by kzar

So far this is a Core issue, but with a change landed to Platform (adblockpluschrome) and therefore the release milestone set...

Jon, please could you change this to be a Platform issue, then file a new issue for the Core change, and update your review to use that issue number?

comment:19 Changed 10 months ago by kzar

The change already pushed to adblockpluschrome doesn't seem to work. I get an error about a missing jsdoc.conf file. Your review for the similar adblockpluscore change adds that file, did you forget to stage it here?

comment:20 follow-up: Changed 10 months ago by kzar

Also, this issue hasn't been triaged yet, and doesn't detail how it's supposed to work. For example, your change attempts to put the docs in adblockpluschrome/docs, where as the existing script you mention puts them in adblockpluschrome/buildtools/docs. Which is correct?

comment:21 in reply to: ↑ 20 Changed 10 months ago by erikvold

Replying to kzar:

For example, your change attempts to put the docs in adblockpluschrome/docs, where as the existing script you mention puts them in adblockpluschrome/buildtools/docs. Which is correct?

Yes the idea is to get rid of the buildtools dir.

comment:22 Changed 10 months ago by kzar

Well, then please can you update the issue description to mention that, and any other relevant details?

comment:23 follow-up: Changed 10 months ago by jsonesen

  • Component changed from Core to Platform
  • Description modified (diff)

Well, then please can you update the issue description to mention that, and any other relevant details?

Absolutely sorry, I see now it is very sparse.

We ​have that already. However, currently sitescripts is calling build.py docs. This issue is about doing that through an npm script instead, and eventually running it from GitLab CI.

I think he means it would be nice to also have for the next branch which I agree (unless I am mistaken and that can be found somewhere)

The ​change already pushed to adblockpluschrome doesn't seem to work. I get an error about a missing jsdoc.conf file. Your review for the similar adblockpluscore change adds that file, did you forget to stage it here?

Yikes, my bad that was a big mistake. Also, thinking about this makes me think I should have it output to ./buildtools/docs until that is all worked out.

comment:24 Changed 10 months ago by sebastian

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:25 Changed 10 months ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:26 in reply to: ↑ 23 Changed 10 months ago by sebastian

Replying to kzar:

I get an error about a missing jsdoc.conf file. Your review for the similar adblockpluscore change adds that file, did you forget to stage it here?

Yeah, I see the file jsdoc.conf in the review but not in the commit. Any reason we didn't add the missing file in a follow-up commit yet?

Replying to jsonesen:

Also, thinking about this makes me think I should have it output to ./buildtools/docs until that is all worked out.

The whole point of this change is to move away from buildtools. So why should the generated docs still go into buildtools/docs? It seems putting them into docs (under the top-level) as the current patch does is exactly what we want, or do I miss something?

comment:27 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 7103 - Add missing jsdoc.conf

comment:28 Changed 10 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:29 Changed 10 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:30 Changed 10 months ago by mjethani

  • Summary changed from Add docs npm script to adblockpluscore and adblockpluschrome to Add docs npm script to adblockpluschrome

comment:31 follow-up: Changed 10 months ago by sebastian

The next step is to integrate those changes with GitLab CI, running npm run docs on every commit in master and uploading the docs to the website.

Tristan, can you help with this or provide some guidance?

comment:32 in reply to: ↑ 31 Changed 10 months ago by tlucas

Replying to sebastian:

The next step is to integrate those changes with GitLab CI, running npm run docs on every commit in master and uploading the docs to the website.

Tristan, can you help with this or provide some guidance?

Yes, i'd be happy to help with this, in case this is not too urgent, i'd tackle this after my bigger release-project.

comment:33 Changed 10 months ago by jsonesen

Manish raised a good point during the review for the addition of jsdocs to core, the configuration file should probably be hidden since this is the convention used for other configurations (with the exception of package.json) what do you think?

comment:34 Changed 3 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from reopened to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.