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

Fix default maxLength and update deps #13

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Fix default maxLength and update deps #13

merged 1 commit into from
Jul 9, 2018

Conversation

jacobheun
Copy link
Collaborator

There are two commits in here; a fix and an update for the dependencies. Happy to separate them into separate PRs if you'd like.

Default maxLength

Currently the maxLength is only being specified if the param is not given to readFixedMessage or readVarintMessage . The problem with that is that both functions are private and they are always called with maxLength, even if it's undefined. I set the default maxLength on the opts prior to those functions being called so it always exists. I also removed the, maxLength is the callback, check inside those methods, as it's not needed since they are private and we always supply the maxLength.

String literals were being passed as errors, which also breaks linting, so I turned those into actual errors and updated the tests.

Dependency Updates

The major update here is to the latest version of Aegir, which required npm script updates.
It also includes the latest pull-reader (1.3.0), which adds the support needed for #8.

@dignifiedquire
Copy link
Owner

Thanks, can you rebase on master please, there are conflicts after I merged
#15

fix: revert package update
fix: remove deprecate warning in tests
chore: update deps
@jacobheun
Copy link
Collaborator Author

@dignifiedquire good to go!

@dignifiedquire dignifiedquire merged commit 1963b21 into dignifiedquire:master Jul 9, 2018
@dignifiedquire
Copy link
Owner

Thanks :octocat:

@jacobheun jacobheun deleted the fix/default-maxlength branch July 9, 2018 23:21
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.

2 participants