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

Handle invalid URL and invalid HTML input #152

Merged
merged 1 commit into from Mar 23, 2016

Conversation

gconnolly
Copy link
Contributor

Fx #151

Description

Handle null reference exception that occurs as a result of a source that is neither valid URL or HTML

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary unnecessary
  • Added myself / the copyright holder to the AUTHORS file does not exist

@@ -101,10 +101,13 @@ function Xray() {
var $ = load(html, url);
node.html($, next);
});
} else {
} else if (source) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This source value is set to null in the params function when it does not pass either the isURL or isHTML test.

@Kikobeats
Copy link
Contributor

You need to merge master branch and resolve conflicts 😄

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.087% when pulling 8b7a0b2 on gconnolly:master into 01726b8 on lapwinglabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.087% when pulling af1e55d on gconnolly:master into 01726b8 on lapwinglabs:master.

@@ -105,10 +105,13 @@ function Xray () {
var $ = load(html, url)
node.html($, next)
})
} else {
} else if (source) {
// `url` is probably HTML
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think that we can avoid this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not my comment, but yeah, at this point the source is almost certainly HTML. I would remove it or possibly reword it. Because source is set to null in the params function, it is not entirely obvious here why source is null, so there might be some value in a comment.

I will leave it to you to decide, let me know and I will squash a change into my commit

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I was thinking, please delete it 😄

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.087% when pulling 1f7c920 on gconnolly:master into 01726b8 on lapwinglabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.087% when pulling 61738eb on gconnolly:master into 01726b8 on lapwinglabs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.087% when pulling 61738eb on gconnolly:master into 01726b8 on lapwinglabs:master.

@Kikobeats Kikobeats merged commit 693d32d into matthewmueller:master Mar 23, 2016
@Kikobeats
Copy link
Contributor

Thanks @gconnolly !

I'm waiting @matthewmueller enable greenkeeper integration to ship v2.1. I hope that for this week 😄

@gconnolly
Copy link
Contributor Author

Thank you @Kikobeats

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

3 participants