Skip to content

Make API path resource configurable via pathPrefix#65

Closed
lenage wants to merge 0 commit intoyahoo:masterfrom
lenage:master
Closed

Make API path resource configurable via pathPrefix#65
lenage wants to merge 0 commit intoyahoo:masterfrom
lenage:master

Conversation

@lenage
Copy link
Contributor

@lenage lenage commented Mar 30, 2015

In this PR,

I added an global var as default pathPrefix(/resource/), and provide an option this.pathPrefix to make it configurable.

NOTE: if you want to remove /resource/ prefix in path, please set this.pathPrefix to ''.

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@Vijar Vijar added the ready label Mar 30, 2015
@yahoocla
Copy link

CLA is valid!

@Vijar
Copy link
Contributor

Vijar commented Mar 30, 2015

@lenage I talked to the team and we're comfortable with just getting rid of the whole /resource/ prefix. Can you make that change?

@lenage
Copy link
Contributor Author

lenage commented Mar 30, 2015

sure,will update code soon,Thanks 


Best Regards,
Yuan He

On Tue, Mar 31, 2015 at 2:36 AM, Rajiv Tirumalareddy
notifications@github.com wrote:

@lenage I talked to the team and we're comfortable with just getting rid of the whole /resource/ prefix. Can you make that change?

Reply to this email directly or view it on GitHub:
#65 (comment)

@lenage
Copy link
Contributor Author

lenage commented Mar 31, 2015

@Vijar I have removed the 'resource/' prefix from url, maybe release a new minor version for it?

@Vijar
Copy link
Contributor

Vijar commented Mar 31, 2015

@lenage This should be backwards compatible since only fetcher.client.js should be relying on fetcher routes directly so a patch version should be fine.

But, can you also remove /resource/ from the fetcher.js file, in the middleware function?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.82% when pulling e233db0 on lenage:master into db2cdaf on yahoo:master.

@lenage
Copy link
Contributor Author

lenage commented Apr 1, 2015

@Vijar Removed and passed the test

@Vijar
Copy link
Contributor

Vijar commented Apr 1, 2015

@lenage This looks good.

I think just to be safe, this will be a minor bump.

However before merging your PR, I want to fix #66 and bump patch. Then, your PR, and bump minor.

@Vijar
Copy link
Contributor

Vijar commented Apr 2, 2015

@lenage looks like this is out of date. If you can fix the conflicts, we can merge this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.25%) to 80.5% when pulling e58daeb on lenage:master into 9cecc83 on yahoo:master.

@lenage
Copy link
Contributor Author

lenage commented Apr 3, 2015

@Vijar conflicts fixed, but this is another issue when do GET, / will be added in resource name, see e58daeb, i hope this is a better idea to handle it

Copy link
Contributor

Choose a reason for hiding this comment

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

@lenage there is always a leading slash before the resource name so the if/else you added below isn't necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i think so, i was remove the if/else before, but the test did not pass. here is the log:

1) Server Fetcher #middleware #GET should respond to GET api request:
     Error: Fetcher "ock_service" could not be found
      at Function.Fetcher.getFetcher (/Users/lenage/Projects/github/fetchr/libs/fetcher.js:78:19)
      at Function.Fetcher.single (/Users/lenage/Projects/github/fetchr/libs/fetcher.js:175:31)
      at /Users/lenage/Projects/github/fetchr/libs/fetcher.js:147:21
      at Context.<anonymous> (/Users/lenage/Projects/github/fetchr/tests/unit/libs/fetcher.js:272:17)
      at Test.Runnable.run (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runnable.js:233:15)
      at Runner.runTest (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:387:10)
      at /Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:470:12
      at next (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:312:14)
      at /Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:322:7
      at next (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:257:23)
      at Object._onImmediate (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:289:5)
      at processImmediate [as _immediateCallback] (timers.js:354:15)

  2) Server Fetcher #middleware #GET should respond to GET api request with custom status code:
     Error: Fetcher "ock_service" could not be found
      at Function.Fetcher.getFetcher (/Users/lenage/Projects/github/fetchr/libs/fetcher.js:78:19)
      at Function.Fetcher.single (/Users/lenage/Projects/github/fetchr/libs/fetcher.js:175:31)
      at /Users/lenage/Projects/github/fetchr/libs/fetcher.js:147:21
      at Context.<anonymous> (/Users/lenage/Projects/github/fetchr/tests/unit/libs/fetcher.js:316:17)
      at Test.Runnable.run (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runnable.js:233:15)
      at Runner.runTest (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:387:10)
      at /Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:470:12
      at next (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:312:14)
      at /Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:322:7
      at next (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:257:23)
      at Object._onImmediate (/Users/lenage/Projects/github/fetchr/node_modules/mocha/lib/runner.js:289:5)
      at processImmediate [as _immediateCallback] (timers.js:354:15)
......

ock_service, any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

In tests/unit/libs/fetcher.js make sure to replace all instances of /resource/ with /. That should solve our problems.

@Vijar
Copy link
Contributor

Vijar commented Apr 5, 2015

@lenage I see the commit on your fork, but for some reason I can't see the changes in this PR's diff...

Maybe squash these commits together into one and do another PR?

@lenage
Copy link
Contributor Author

lenage commented Apr 7, 2015

@Vijar Here is new PR for this changes #69

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.

4 participants