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

Add type and characters options #4

Merged
merged 11 commits into from May 8, 2019
Merged

Conversation

stroncium
Copy link
Contributor

@stroncium stroncium commented Apr 15, 2019

Added type and character options, fixes #2

@sindresorhus sindresorhus changed the title Added type and character options, solves #2 Add type and character options Apr 19, 2019
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

It's a little bit awkward to have two mutually exclusive options, but I couldn't think of a better way to handle it. Can you?

@stroncium
Copy link
Contributor Author

@sindresorhus If it was me, I'd just do exports = { xxx: function } instead of exports = function in the first place, but now that it is the way it is I don't think making a breaking change out of this feature is a good idea.

I do think that that using function.function is misleading, so it's probably best to leave it as it is.
I don't think it will be confusing for users in reality, it's hard to expect someone using anything but constants for this parameters and the intent it passed clear enough. For the purpose of code maintenance, after this update the library functionality will be full(in terms of nothing to add except maybe additional predefined character sets), so it should be ok too.

@stroncium
Copy link
Contributor Author

stroncium commented Apr 19, 2019

re: characters string length
forgot to check it before. 65536 is a sane limit(all characters javascript supports) even though technically library supports using using same character multiple times allowing for changes in distribution. I don't expect anyone in lifetime of this library to actually use this feature and require a length of more than 65536, and cutting it to 65536 allows us to require less entropy and run faster.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated

Type: `string`

Setting this option makes it select characters from the string. Can not be set at the same time as `type`. Maximum length 65536.
Copy link
Owner

Choose a reason for hiding this comment

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

Should we have a minimum amount of characters? Doesn't really make sense to only have two characters, for example. Can you also mention the entropy aspects of the amount of characters to choose? And a recommended minimum maybe?

Copy link
Contributor Author

@stroncium stroncium May 6, 2019

Choose a reason for hiding this comment

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

Don't see any problem with strings length 2. Why not have random string of 0 and 1 for example. Length 0 isn't computable though, so obviously is a limit. Length 1 technically is valid, though isn't random, but to prevent this type of user errors we also need to check unique glyphs, which IMO is a bit of overkill.

Can you also mention the entropy aspects of the amount of characters to choose?

Don't really understand what are you pointing at. If user doesn't know what entropy is we need to include whole lecture on information theory, and if he knows what entropy is then there is nothing to explain.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add type and character options Add type and characters options May 4, 2019
Yanis Benson and others added 2 commits May 6, 2019 05:42

test('argument errors', t => {
t.throws(() => {
cryptoRandomString(Infinity);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one should we use though, with brackets and separate line, or without? In execa I was suggested the opposite changes and they were accepted in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record I myself is in favor of brackets+line for better intent indication and readability.

Copy link
Owner

Choose a reason for hiding this comment

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

Which one should we use though, with brackets and separate line, or without?

This format. Like you said, it makes the intention clearer. I do this for all code. I only use inline arrow functions when they are meant to return something.

In execa I was suggested the opposite changes and they were accepted in the end.

That must have been an oversight. Fixed: sindresorhus/execa@f6db264

@sindresorhus sindresorhus merged commit 51402ba into sindresorhus:master May 8, 2019
@sindresorhus
Copy link
Owner

Looks good 👌

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.

Support more types of characters
2 participants