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

Throw better errors when there are parsing issues #49

Merged
merged 3 commits into from
May 9, 2020

Conversation

xavdid
Copy link
Collaborator

@xavdid xavdid commented May 9, 2020

This is both a little more descriptive about where the error is raised, plus actually broadcasts the error contents. I added a test with a broken version of alice in wonderland (there's an invalid & in a chapter title)

While I was in there, I added the files property so we don't ship tests and the epub files in the npm package. Drops the package size pretty dramatically.

test/test.ts Outdated
@@ -27,4 +33,19 @@ mocha.describe('EPub', () => {
var res = epub.walkNavMap(branch, [], []);
assert.ok(res);
});

mocha.it('raises descriptive errors', () => {
// const epub = new EPub('./example/alice.epub')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what's weird is that if this line is uncommented instead of the below, the tests still pass when they shouldn't 🤔 not sure if I'm parsing it wrong or what

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, mocha doesn't wait for callbacks to be executed, does it?

Copy link
Owner

Choose a reason for hiding this comment

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

We should move to async/await... :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, you're right. I haven't written mocha tests w/ done for a while.

But you're also right that we should use promises, so I added a tool for awaiting events and now it works like a charm!

test/test.ts Outdated
mocha.it('basic parsing', () => {
const epub = new EPub('./example/alice.epub');

epub.on('end', ()=> {
assert.ok(epub.metadata.title)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added some basic coverage

@@ -19,6 +19,10 @@
"type": "git",
"url": "http://github.com/julien-c/epub.git"
},
"files": [
"epub.js",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are the only two files actually needed at runtime for the user

Copy link
Owner

Choose a reason for hiding this comment

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

good idea

@xavdid xavdid requested review from julien-c and andris9 May 9, 2020 05:34
@xavdid
Copy link
Collaborator Author

xavdid commented May 9, 2020

ok, we should be good here! Added extra tests. My editor also removed trailing whitespace (which makes the div look extra big, but it's not). let me know if that's an issue. :)

@julien-c
Copy link
Owner

julien-c commented May 9, 2020

lgtm, feel free to merge, thanks for improving this:)

@xavdid xavdid merged commit 09a5fdf into master May 9, 2020
@xavdid xavdid deleted the better-error-messages branch May 9, 2020 18:19
@xavdid
Copy link
Collaborator Author

xavdid commented May 9, 2020

sweet! can you release to npm when you have a chance?

@julien-c
Copy link
Owner

julien-c commented May 9, 2020

hmm, I think you released last time, no?

@julien-c
Copy link
Owner

julien-c commented May 9, 2020

(if not the case I can add you as a maintainer on npm, if that works for you)

@xavdid
Copy link
Collaborator Author

xavdid commented May 9, 2020

Oh, so I am. I had forgotten. 😁 I'll take care of that!

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

2 participants