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

Remove wordlists from index export #28

Closed
wants to merge 6 commits into from
Closed

Remove wordlists from index export #28

wants to merge 6 commits into from

Conversation

weilu
Copy link
Contributor

@weilu weilu commented Oct 24, 2015

With this change set, there's no longer any default word list. Wordlist become a required argument for all functions previously uses the english wordlist as the default. The special check on Japanese wordlist is replaced by the optional delimiter argument, which default to a regular space.

Also added type checking on the affected functions, alongside basic JSDoc.

This change set is based on discussion in #24

@@ -20,16 +20,16 @@ However, there should be other checks in place, such as checking to make sure th

```javascript
var bip39 = require('bip39')
var wordlist = require('bip39/wordlists/en')
Copy link
Contributor

Choose a reason for hiding this comment

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

Preferred over bip39/en or bip39/english?

@dcousens
Copy link
Contributor

ACK, LGTM, just 1 nit which I'm not really phased about, but, IMHO might look nicer by default.

We could maybe autobind the word list for each word list import?

bip39/english being bip39 bound with bip39/wordlists/en?

@weilu
Copy link
Contributor Author

weilu commented Oct 26, 2015

How do you autobind?

@dcousens dcousens closed this Oct 6, 2016
@dcousens dcousens deleted the wordlist-export branch October 6, 2016 03:00
@dcousens dcousens added this to the 3.0.0 milestone Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants