criso / fbgraph

NodeJs module to access the facebook graph api
http://criso.github.io/fbgraph/
1.09k stars 176 forks source link

Added the ability request graph paths which already have the graph-prefix #14

Closed cimnine closed 11 years ago

cimnine commented 11 years ago

With this pull it should get easier to fetch data.paging._next and data.paging._previous, which provide 'full' urls, and not relative paths.

~Chris

criso commented 11 years ago

Awesome! please add a quick unit test, and we're good!

cimnine commented 11 years ago

Because I never wrote a Vows before, would that be right?

      // just after "and requesting an api url with a missing slash"

      "and requesting an api url with prefixed graphurl": {
        topic: function() {
          graph.get(graph.getGraphUrl() + "/zuck/picture");
        },

        "should be able to get valid data": function (err, res) {
          assert.include(res, "image");
          assert.include(res, "location");
        }
      },

      // and before "and trying to access data that requires an access token"
criso commented 11 years ago

That would work. But you don't need to test it like that. Just test that passing a string without and with "http(s)" to prepareUrl returns the right thing.

cimnine commented 11 years ago

I'll do that, but then cleanUrl() would require some separate tests, too, wouldn't it? Shall I write some for it too?

Update: Is that possible, given that prepareUrl is a method of Graph and not exported?

criso commented 11 years ago

Sure. The test would be written similar to this:

"Before starting a test suite": {
    topic:  function () {
      return graph.setAccessToken(null);
    },

    "*Access Token* should be null": function (graph) {
      assert.isNull(graph.getAccessToken());
    },

- Check out the first batch section under graph.test.js - you should be able to just add another test to that.
Something like

```javascript
// after    "should be able to set *request* options": function (graph) " section 

"clean url should be clean": function (graph) {
      assert.isEqual(graph.cleanUrl(url), expectedUrl);
    },
cimnine commented 11 years ago

Yes, but setAccessToken() is exported, and prepareUrl() is not, and cleanUrl() neither. This is, because they both are methods of the Graph class, which does not get exported either. That is why I can't figure out how to write a direct test specific for prepareUrl() (or cleanUrl()), and not only an indirect one as proposed earlier.

// Declaration of Graph & ctor for Graph
// IMHO **not** accessible from outer module
function Graph(method, url, postData, callback) {
  // ...
}

// Method cleanUrl() of Graph
// IMHO **not** accessible from outer module
Graph.prototype.cleanUrl = function(url) {
  //...
}

// Method prepareUrl() of Graph
// IMHO **not** accessible from outer module
Graph.prototype.prepareUrl = function(url) {
  // ...
}

// For example setGraphUrl()
// Accessible from outer module
exports.setGraphUrl = function (url) {
  // ...
}

Am I wrong?

criso commented 11 years ago

Nope. You're right. My bad. I'll merge the pull request later today, so that I'll remember to npm publish. Thanks!

cimnine commented 11 years ago

So you're adding the test I proposed or shall I push it and add it to the request?

criso commented 11 years ago

Yup, please add your proposed test.

cimnine commented 11 years ago

Sry, the test fails. Fix will arrive soon.