Skip to content

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

Merged
merged 3 commits into from Oct 18, 2012

2 participants

@cimnine
cimnine commented Oct 9, 2012

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

@cimnine cimnine Added the ability request graph paths which already have the graph-pr…
…efix

This is makes it easier to fetch `data.paging._next` and `data.paging._previous`, which provide 'full' urls, and not relative paths.
79bcb9b
@criso
Owner
criso commented Oct 10, 2012

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

@cimnine
cimnine commented Oct 10, 2012

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
Owner
criso commented Oct 10, 2012

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
cimnine commented Oct 11, 2012

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
Owner
criso commented Oct 11, 2012

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
cimnine commented Oct 11, 2012

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
Owner
criso commented Oct 11, 2012

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

@cimnine
cimnine commented Oct 11, 2012

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

@criso
Owner
criso commented Oct 11, 2012

Yup, please add your proposed test.

@cimnine
cimnine commented Oct 11, 2012

Sry, the test fails. Fix will arrive soon.

@criso criso merged commit 4340999 into criso:master Oct 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.