Opened 12 months ago

Closed 7 weeks 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 intergrating this with GitLab CI/CD. Perhaps Tristan can give some input on that? Also what is about the docs for adblockpluscore?

Version 0, edited 12 months ago by sebastian (next)

comment:6 Changed 12 months ago by mjethani

Makes sense to me.

comment:7 follow-up: Changed 9 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 9 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:9 Changed 9 months ago by abpbot

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

comment:10 Changed 9 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:11 Changed 9 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 9 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 9 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 9 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 9 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 9 months ago by tlucas

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

comment:17 Changed 9 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 9 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 9 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 9 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 9 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 9 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 9 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 9 months ago by sebastian

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

comment:25 Changed 9 months ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:26 in reply to: ↑ 23 Changed 9 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 9 months ago by abpbot

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

comment:28 Changed 9 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:29 Changed 9 months ago by jsonesen

  • Review URL(s) modified (diff)

comment:30 Changed 9 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 9 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 9 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 9 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 7 weeks 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.