Skip to content

Object.keysToArray documentation #114

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

Closed

Conversation

jmagaram
Copy link
Contributor

@jmagaram jmagaram commented Mar 20, 2023

Object.keysToArray documentation. Why not just call this "keys"? Shorter and matches Javascript. I see a few places in the code with keysToArray - should change it everywhere. Map just uses keys and values.

Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

Looking great overall! Just 2 minor things.

Comment on lines 31 to 33
## Specifications
- [ECMAScript Language Specification](https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-object.keys)
- [Object.keys on MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We've not used a separate subtitle for specifications before, but rather just linked to MDN on a separate line, like "See Object.keys on MDN". I like your subtitle and I think we should consider doing that instead further down the line. So, I propose we change this to follow how we've done the rest ("See Object.keys on MDN"), and open a separate issue where we can discuss whether we should move everything to a separate subtitle.

Sounds good?

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 don't know where I got that. I'll change it everywhere. One issue is that in the Intellisense pop up there isn't much space and the references can cause the examples to scroll off the screen. So sometimes I'll add it to the summary sentence to reduce vertical scroll and maybe other times as a separate paragraph. The "Examples" header seems too big in that pop up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I'll do most places.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to change "keysToArray" to "keys" everywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep the original naming for now, and we can revisit later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm struggling a bit to see the rationale for keeping the name as-is, given that the foremost guiding principle is familiarity for JS developers. This naming is also inconsistent with just about everything else in the library. There is no other function with a ToArray suffix.

@jmagaram
Copy link
Contributor Author

Consolidated into #117

@jmagaram jmagaram closed this Jul 12, 2023
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.

3 participants