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: add jsdoc types #79

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

helmturner
Copy link

@helmturner helmturner commented Jun 2, 2023

Here is the current progress on my light-weight refactor with JSDoc types, as mentioned in #29

@TimothyJones
Copy link
Member

This is a nice set of changes.

Are the typescript and @types dependencies needed? I guess you need those to parse tsdoc?

What does this need to be out of draft?

@helmturner
Copy link
Author

helmturner commented Jun 2, 2023

Yeah,TS is still doing all the work - it's just pulling types from the comments, instead.

I put it up as a draft because I wasn't sure if I should leave it as is (i.e. leave the errors and skip type checking during CI) or wait until I make the refractors necessary to eliminate the errors.

@helmturner
Copy link
Author

No idea what I was thinking using "style" for one commit but "chore" for the rest 😅

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2023

Codecov Report

Merging #79 (b8f30e4) into master (2e3f60a) will decrease coverage by 0.08%.
The diff coverage is 85.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
- Coverage   97.57%   97.50%   -0.08%     
==========================================
  Files          31       31              
  Lines        1280     1281       +1     
==========================================
  Hits         1249     1249              
- Misses         31       32       +1     
Impacted Files Coverage Δ
lib/configuration.js 94.73% <ø> (ø)
lib/detect-package-manager.js 100.00% <ø> (ø)
lib/format-commit-message.js 100.00% <ø> (ø)
lib/latest-semver-tag.js 100.00% <ø> (ø)
lib/lifecycles/commit.js 96.96% <ø> (ø)
lib/preset-loader.js 100.00% <ø> (ø)
lib/run-exec.js 100.00% <ø> (ø)
lib/run-execFile.js 81.81% <ø> (ø)
lib/stringify-package.js 100.00% <ø> (ø)
lib/updaters/index.js 71.79% <0.00%> (-1.89%) ⬇️
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TimothyJones
Copy link
Member

My thinking is the best strategy would be to merge in small changes that are solid improvements - under that strategy, I think working type checking enabled in CI is a stronger change (even if only for a few files). What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants