Skip to content

Conversation

@michael-elbert
Copy link

PLEASE review my code. It works but I'm afraid it's a bit hacky.

michael-elbert and others added 2 commits October 12, 2016 12:27
PLEASE review my code. It works but I'm afraid it's a bit hacky.
}


var x2js = new X2JS();
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see where this is coming from

return
}

/*
Copy link
Owner

Choose a reason for hiding this comment

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

please don't leave commented code, version control exists for a reason

complete: function(result){
var jsonResult = x2js.xml_str2json(result.responseText);
//console.log(JSON.stringify(jsonResult));
callback(jsonResult);
Copy link
Owner

Choose a reason for hiding this comment

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

you changed the type of the argument that is passed to callback (it used to be the xhr object) and didn't fix all instances where this function is used (namely the migrate/progress call in mule.js)

@@ -1,3 +1,4 @@

Copy link
Owner

Choose a reason for hiding this comment

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

all of the whitespace changes are uncalled for. also use tabs for indentation (in all the other places)

} else {
data.query.results.Chars = d
}

Copy link
Owner

Choose a reason for hiding this comment

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

These switches are presumably for compatibility with yql? If you intend to have it as a fallback, actually do it.

if (this.overlay) this.overlay.hide()

var d = data.query.results.Chars
console.log(data);
Copy link
Owner

Choose a reason for hiding this comment

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

no

rld.text('\u21bb')
if (mule.data) {
var updated = new Date(mule.data.query.created)
var updated = new Date(mule.data.created)
Copy link
Owner

Choose a reason for hiding this comment

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

the query object and it's created property are inherent to the yql api. With your changes this is same as new Date() which is not correct if loading from cache. You need to set this timestamp yourself after updating.

@michael-elbert
Copy link
Author

Like I said, it was just a hacky example of getting rid of YQL, not intended to be shipped out immediately. I didn't mean to leave my code so ugly but I just wanted you to see an alternative. I didn't exactly go through the entire code to look for syntax, I was just trying to make it workable. I'm not the greatest at just picking up a project out of the blue and making glorious edits, sorry about that. But yeah, I just wanted to give an example of life without the Yahoo API.

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