Splited declarations + added params jsdoc for intellisense. #5
Conversation
| * Compile a Pug template and render it with locals to html string. | ||
| * @param {(Options & LocalsObject)} options Pug Options and rendering locals |
There was a problem hiding this comment.
Can you please move the @param into the one comment above and remove the duplicate type declaration (it's not needed)?
There was a problem hiding this comment.
@blakeembrey the type is not needed, but at least for typedoc you need to have different param descriptions for different overloads afaik (you can click through the overloads)
There was a problem hiding this comment.
There has to be comments section for each overload declarations to have intelisense on overloads:
#4 (comment)
There was a problem hiding this comment.
@felixfbecker Ok, cool - so I guess we have to duplicate the params?
I'd still love to figure out how to not duplicate things. I don't think it's that important to have docs on every single overload as long the main one you see does.
There was a problem hiding this comment.
Ok, just took a look and yeah, that sucks. I'm going to submit an issue to TypeScript if it's not already tracked. Surprised I hadn't noticed this before really.
There was a problem hiding this comment.
Typings has to be usefull, not look nice on github repository 😜 It's better to duplicate than have no info about params or function you are using.
There was a problem hiding this comment.
It's not about looking nice, and it is already useful. It's more about maintaining something. Although this isn't a big use-case, there's dozens of other people with huge definitions who are trying to maintain them. If it takes a cumulative 5 minutes extra each time to update all definitions because we repeated ourselves, that's 100s of hours over a long enough duration and, of course, the probability that each change could introduce a bug because a new contributor doesn't realise duplication is required or we forget ourselves and become inconsistent.
I found and commented on microsoft/TypeScript#407.
There was a problem hiding this comment.
In any case, it's the same reason you wouldn't duplicate the type arguments into each param documentation. It's n*2 locations you need to fix every time.
There was a problem hiding this comment.
Are you gonna merge this pull request? We can wait a few months until TypeScript team solve this problem.
There was a problem hiding this comment.
There's 1000 other definitions where this isn't used. I don't think it's a valuable addition, as anyone using the tooling would understand how this works today, and the future improvements from TypeScript will also be a welcome addition.
However, the issue here is also that the types need to be removed from JSDoc.
| @@ -84,7 +110,7 @@ declare namespace pug { | |||
| /** | |||
| * An object that can have multiple properties of any type. | |||
| */ | |||
| export interface AnyObject { | |||
| export interface LocalsObject { | |||
There was a problem hiding this comment.
This seems like an unnecessary breaking change to me
There was a problem hiding this comment.
AnyObject isn't meaningful - you don't know what is the function of object you are passing to as a param of render() or other function. LocalsObject means it's the object holding locals variables to render the view from template.
|
Closing as I made a PR to npm @types repo: |
Fixed #4 intellisense issue.