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

chore: exclude services from docs #6791

Merged
merged 2 commits into from
Jul 25, 2021

Conversation

SethFalco
Copy link
Contributor

Related: #6677 (comment)

It was suggested to use a path like services/**/*.service.js, however I'm taking a different approach for 2 reasons:

  • I'm using excludePattern, as it appears to me exclude doesn't support wildcards. (More Info)
  • It checks for .js instead of .service.js because the example files in the comment didn't end with .service.js, and wouldn't have been excluded otherwise.

@shields-ci
Copy link

shields-ci commented Jul 22, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @SethFalco!

Generated by 🚫 dangerJS against 8eda31e

@calebcartwright calebcartwright added developer-experience Dev tooling, test framework, and CI documentation Developer and end-user documentation labels Jul 23, 2021
@calebcartwright
Copy link
Member

it checks for .js instead of .service.js because the example files in the comment didn't end with .service.js, and wouldn't have been excluded otherwise.

I'm going to have to defer to others on this, but my initial reaction is that pattern could be too encompassing. It does make sense to exclude the individual service classes, but there are other files that I could see having docs for being useful for contributors (e.g various helper functions)

@SethFalco
Copy link
Contributor Author

SethFalco commented Jul 23, 2021

Unfortunately, not that I'm aware of.
The only read-only API would be the unauthenticated API.

I did just check, and I see nothing in the documentation about it, and on the API key page the only option is "Regenerate API Key" which has no other options, it'll just revoke the old key for a new one.

Edit: Whoops... too early in the morning for me. Posted that on the wrong issue.

@chris48s
Copy link
Member

Yeah I think I'd be inclined to go with something like

"excludePattern": ".+\\.service\\.js$"

We do have some "base" files under /services that contain helpers used by quite a few other badges. We haven't really done it (yet), but I think there is value to documenting those and including them in the docs. I wouldn't want exclude docstrings we add to files like
https://github.com/badges/shields/blob/master/services/npm/npm-base.js or
https://github.com/badges/shields/blob/master/services/github/github-api-provider.js
for example.

On reflection, I think the existing files with docstrings under /services do actually match this pattern of being docs for shared helpers not individual services.

@SethFalco
Copy link
Contributor Author

Sure, I've updated it to exclude all *.service.js files regardless of parent directory.


We currently don't have any docs in *.spec.js files or *.tester.js files, but could it be worth adding those to ignore as well for future reference?

Something like the following:
.+\\.(service|spec|tester)\\.js$

At the very least, it gives build-docs fewer files to process, but performance isn't a significant issue and there aren't any reasons to use JSDocs on those files, I don't think.

@calebcartwright
Copy link
Member

We do have some "base" files under /services that contain helpers used by quite a few other badges. We haven't really done it (yet), but I think there is value to documenting those and including them in the docs

Agreed

We currently don't have any docs in *.spec.js files or *.tester.js files, but could it be worth adding those to ignore as well for future reference?

Sounds reasonable to me

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

This LGTM, will hold off on merging to see what Chris thinks though

@chris48s
Copy link
Member

yep - agreed 👍

@repo-ranger repo-ranger bot merged commit 1e72d5e into badges:master Jul 25, 2021
@SethFalco SethFalco deleted the exclude-services-docs branch July 25, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI documentation Developer and end-user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants