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

refactor(path): always name the parameters (add param definition check in doc linter) #6158

Merged
merged 5 commits into from
Oct 30, 2024

Conversation

efekrskl
Copy link
Contributor

@efekrskl efekrskl commented Oct 29, 2024

Closes #5439

This implementation doesn't go well with unnamed parameters (see the other changes in path/*). IMO, this is ok. As named parameters and Jsdoc param names go hand in hand, especially considering these are public APIs. Also, immediately on implementation, this helped me uncover some unused fields in the same functions. I'm curious on your opinions on this.

image

@efekrskl efekrskl requested a review from kt3k as a code owner October 29, 2024 17:47
@github-actions github-actions bot added the path label Oct 29, 2024
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.52%. Comparing base (24cd7cf) to head (ab7bf60).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6158   +/-   ##
=======================================
  Coverage   96.52%   96.52%           
=======================================
  Files         536      536           
  Lines       40765    40769    +4     
  Branches     6118     6118           
=======================================
+ Hits        39347    39351    +4     
  Misses       1374     1374           
  Partials       44       44           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@efekrskl efekrskl changed the title feat: add param definiton check feat: add param definition check Oct 29, 2024
@kt3k kt3k changed the title feat: add param definition check refactor(path): always name the parameters (add param definition check in doc linter) Oct 30, 2024
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM. Nice! Thanks

@@ -26,8 +26,9 @@ export type { GlobOptions };
*/
export function joinGlobs(
globs: string[],
{ extended = true, globstar = false }: GlobOptions = {},
Copy link
Member

Choose a reason for hiding this comment

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

Good to find unused parameter 👍

@kt3k kt3k merged commit 2d9e212 into denoland:main Oct 30, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to-do: add doc check for JSDoc tags that aren't needed
2 participants