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

Convenience methods #155

Closed
wants to merge 6 commits into from
Closed

Conversation

Eneris
Copy link

@Eneris Eneris commented Sep 30, 2021

Hopefully leads to resolving sindresorhus/electron-store#52

@Eneris Eneris marked this pull request as ready for review September 30, 2021 21:52
source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
test/index.ts Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I appreciate the pull request, but it generally needs a bit more work. Try to look over your own diff before submitting a pull request to catch a lot of obvious stuff ;)

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
Martin Kalábek and others added 2 commits October 4, 2021 10:03
Apply some of the typo suggestions

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@Eneris
Copy link
Author

Eneris commented Oct 4, 2021

Thanks for taking a look at this. Since I am using this module a lot, I wanned to give something back


#### .mutate(key, mutation)

Calls supplied mutation on the item and replaces it with its result.
Copy link
Owner

Choose a reason for hiding this comment

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

This sentence needs more work and I think it could be useful with an example showing how to use it.

readme.md Outdated Show resolved Hide resolved

Append value into the array

Type of the item must be `array`. Trying to append into different type will result in TypeError.
Copy link
Owner

Choose a reason for hiding this comment

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

This is missing from the TS docs.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean by "this"?

Append value into the array.

@param {key} - You can use [dot-notation](https://github.com/sindresorhus/dot-prop) in a key to access nested properties.
@param value - Must be JSON serializable. Trying to set the type `undefined`, `function`, or `symbol` will result in a `TypeError`.
Copy link
Owner

Choose a reason for hiding this comment

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

This is missing from the readme.

Copy link
Author

Choose a reason for hiding this comment

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

Explain


if (this._containsReservedKey(key)) {
throw new TypeError(`Please don't use the ${INTERNAL_KEY} key, as it's used to manage this module internal operations.`);
}
Copy link
Owner

Choose a reason for hiding this comment

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

This is used multiple times now and should be abstracted into a function.

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
Toggle a boolean item.

@param key - The key of the item to toggle.
@returns the new value after successful toggle
Copy link
Owner

Choose a reason for hiding this comment

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

Should be properly formatted.

@sindresorhus
Copy link
Owner

The docs generally still needs more work. There are missing things and inconsistencies.

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@sindresorhus
Copy link
Owner

Bump

@Eneris
Copy link
Author

Eneris commented Oct 10, 2022

Well, your comments are mostly vague and carry no clear information about what is wrong or needs to be done... I'd appreciate some more help with this.

EDIT: After some time, this has become more hassle than it's worth doing anything for this module

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