Skip to content

Fix(49472): Added docs for Set and Map types #49522

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

Merged
merged 9 commits into from
Jul 14, 2022

Conversation

Grubba27
Copy link
Contributor

Fixes #49472

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jun 13, 2022
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Reading the rest of the comments, I feel the same way about nearly all of them: Some parts are valuable, the rest is redundant. @DanielRosenwasser or @andrewbranch what do you think about incomplete-but-interesting JSDoc vs complete-but-redundant?

Comment on lines 32 to 33
* @param key The key of the element to add to the Map object. The key may be any JavaScript type (any primitive value or any type of JavaScript object).
* @param value The value of the element to add to the Map object. The value may be any JavaScript type (any primitive value or any type of JavaScript object).
Copy link
Member

Choose a reason for hiding this comment

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

These comments are not very valuable, but I guess they're OK. In particular, "The key may be any javascript type" is generally true, but untrue for set, since K and V are declared on Map and will already be instantiated.

I'd vote to remove these lines, but let me know if you want to keep them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think almost the same. Most of these are MDN docs with some adjustments to make them easier to read while you are coding. Without the need for going to the docs. But if it makes more sense I would be delighted to adjust

Copy link
Member

Choose a reason for hiding this comment

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

Personally I would delete any JSDoc that essentially just rewrites the code (or other in-scope JSDoc comments) as an English sentence. E.g.

@param key The key of the element to remove from the Map object.

  • The key literally just the name of the parameter
  • of the element I think that’s implied
  • to remove just a synonym for delete which is the name of the method
  • from the Map just the name of the type
  • object c'mon 😄

@sandersn sandersn self-assigned this Jun 21, 2022
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Please remove all the redundant comments based on @andrewbranch 's criterion.

@Grubba27
Copy link
Contributor Author

Okay! I've done some adjusts with the jsdocs but I'm still not sure if they are all right especially with these two parts:

    /**
     * Returns a specified element from the Map object. If the value that is associated with the provided key is an object, then you will get a reference to that object and any change made to that object will effectively modify it inside the Map.
     * @param key
     * @returns Returns the element associated with the specified key. If no element is associated with the specified key, undefined is returned.
     */
    get(key: K): V | undefined;

makes sense to have the description and the return part?

also, my other question would be if makes sense having jsdocs like this one:

    /**
     * Removes the specified element.
     * @param key
     */
    delete(key: K): boolean;

because the method is already described itself. I'm a little bit confused

@andrewbranch
Copy link
Member

  1. I think you can just delete @param key if it doesn’t have a description.
  2. For self-explanatory methods like delete, I don’t think it really needs the description, but I don’t have a strong opinion. However, delete should definitely have a @returns since it’s not at all obvious what that boolean means. I think it might read a little weird to have @param or @returns tags with no top-level description, so I’d be ok with leaving Removes the specified element in. @sandersn what do you think?

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

A couple straggling @params but otherwise I think this looks good

Comment on lines 10 to 11
* @param callbackfn
* @param thisArg
Copy link
Member

Choose a reason for hiding this comment

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

@params without a description don’t do anything, I’m pretty sure (same thing on Set)

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks @Grubba27!

@sandersn sandersn merged commit 4902860 into microsoft:main Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

No documentation for Map and Set types
4 participants