Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Fix test cases and docs #104

Merged
merged 2 commits into from
Feb 11, 2020
Merged

Fix test cases and docs #104

merged 2 commits into from
Feb 11, 2020

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Feb 6, 2020

This PR:

  • Updates typedoc of baseTrie get and put to match its type definition (only Buffer).
  • Updates typedoc dep to next to utilize library mode.
  • Updates test cases from strings to buffers for trie keys and values.
  • Updates travis node 6 runner to node 12.
  • Matches default style of badges.

@holgerd77
Copy link
Member

@ryanio Thanks for the PR! If you later on add such significant changes like updating the Node versions in the test setup, please remember to update the PR title as well. Stuff like this is otherwise too likely to be overlooked when writing up release notes.

Will mention @s1na for cc here.

@s1na
Copy link
Contributor

s1na commented Feb 7, 2020

Hey yeah Ryan wrote me. I think we can update the tests to use buffers explicitly as Ryan himself suggests.

@ryanio ryanio changed the title Fix failing travis readme badge Fix test cases and docs Feb 7, 2020
@coveralls
Copy link

coveralls commented Feb 7, 2020

Coverage Status

Coverage increased (+0.05%) to 93.015% when pulling b2c6fe9 on fixReadme into d48a38c on master.

@ryanio
Copy link
Contributor Author

ryanio commented Feb 7, 2020

Thanks @s1na, I have updated the typedoc and test cases, please take a look and let me know if it looks good 🙂

@ryanio ryanio requested a review from s1na February 7, 2020 17:29

# merkle-patricia-tree

## Index
Copy link
Contributor

Choose a reason for hiding this comment

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

So I see quite a few items here that I don't think should be public and documenting them might give the impression they are public. We're exporting them from the individual files so that other files can use, but ideally only the things re-exported in index.ts should be considered public. I don't remember exactly if typedoc supported something like this, I remember I had struggled with this before..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks. In ethereumjs-util I discovered typedoc@next has a library mode (TypeStrong/typedoc#639) that is quite neat. I gave it a try here with some additional exclude params and seems like it's working well. I just pushed a commit if you would like to take a look.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Skimmed through. Looks good, thanks Ryan! It seems the private attributes of the classes are still documented even though you specifiec excludePrivate (maybe we'll have to add a @private or something typedoc can detect). But this can be done in future. I'm not even sure if we're excluding these private stuff in other libraries.

trie.checkpoint()
trie.put('test', 'something', function () {
trie.put('love', 'emotion', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love these test cases lol

'2386bfb0de9cf93902a110f5ab07b917ffc0b9ea599cb7f4f8bb6fd1123c866c': true
d5f61e1ff2b918d1c2a2c4b1732a3c68bd7e3fd64f35019f2f084896d4546298: true,
e64329dadee2fb8a113b4c88cfe973aeaa9b523d4dc8510b84ca23f9d5bfbd90: true,
c916d458bfb5f27603c5bd93b00f022266712514a59cde749f19220daffc743f: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm not sure I like this linting rule, but doesn't matter..

"mdEngine": "github",
"excludeNotExported": true
"gitRevision": "master",
"exclude": "src/**/!(secure|checkpointTrie|baseTrie).ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope they do add a feature to automatically detect public API so we don't have to exclude by file name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree, the library mode is a great first step so hopefully we'll see more in the future in that direction.

@ryanio ryanio merged commit d5861b8 into master Feb 11, 2020
@holgerd77 holgerd77 deleted the fixReadme branch February 11, 2020 22:40
@holgerd77
Copy link
Member

👍

@s1na thanks for settling this out!

@ryanio ryanio mentioned this pull request Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants