-
Notifications
You must be signed in to change notification settings - Fork 130
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(lint): turn on more jsdoc lint rules #580
base: master
Are you sure you want to change the base?
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2ef1042:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #580 +/- ##
==========================================
+ Coverage 98.41% 98.51% +0.09%
==========================================
Files 127 152 +25
Lines 7954 10486 +2532
Branches 724 875 +151
==========================================
+ Hits 7828 10330 +2502
- Misses 123 153 +30
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Let's wait with this PR for after the v2 release. We haven't decided yet if we are adding dataLast impls to guards, and if not it might mean we need to relax the rule to allow some functions not to be tagged with dataFirst and dataLast. |
are the non-datafirst and non-datalast rules fine? i do think require-jsdoc and require-throws are useful to have on (they caught that the |
We'll have to circle back on this once v2 is out so we can see what the state of the library is. I tend to say I prefer them, but I've also noticed that we could simply drop the tags and the duplication of the docblocks and have one documentation block for both... It's a pain to update docs right now as it's just copy-pasting between the 2 versions to fix the example and signature slightly. It was useful when purrying was done manually, but those exceptions are so rare (I think only zipWith) that we're better off just having less optimal docs for that one case. |
yeah, i've been thinking about this too... it's kind of a pain having the docs duplicated. i think tsserver will show the docs for the first overload if the other overloads don't have docs anyway, so it's probably fine, but it would be a change... hm... |
Those are good points to keep the duplication, at least for now. We can consider writing a "linting" script that would check that the dataFirst and dataLast description have the same content and fail the build if they don't, but it also makes sense in some cases to have 2 different descriptions. e.g. we can remove the "pipable" tag (that should be renamed to "lazy" in v2) from the dataFirst impls. |
follow-up #558