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

Content-type parsing #47

Merged
merged 2 commits into from
Mar 17, 2016
Merged

Content-type parsing #47

merged 2 commits into from
Mar 17, 2016

Conversation

skol-pro
Copy link
Contributor

  • Created Content-Type module + factory
  • Refactored resource-http-interceptor factory to use content-type checker

- Refactored resource-http-interceptor factory to use content-type checker
@maennchen
Copy link
Contributor

Could you add the bower & npm dependency?

@skol-pro
Copy link
Contributor Author

See my comment on #46

@maennchen
Copy link
Contributor

Ah, sorry.

  1. You can install it by adding it to the package.json and bower.json.
npm install content-type --save
bower install --save jshttp/content-type
  1. Run those commands at the end:
npm install
npm run compile
  1. I'll write the test for you as a gist.

@skol-pro
Copy link
Contributor Author

Awesome ok, I already ran npm install but I was afraid that it would create useless stuff.
Whatever, will keep that in mind, thank you. I'm just wondering where that "jshttp/" comes from ?

Thank you for the test, will take a look then.

@maennchen
Copy link
Contributor

The jshttp comes from the github username of the author. I'm however afraid, that the package is in fact not compatible with bower.
I'll create a fork which will support it.

@skol-pro
Copy link
Contributor Author

Ooooh ok I see.
By the way, I'm just discovering npm run compile, this is awesome ! Beginning to understand processes.

@skol-pro
Copy link
Contributor Author

Ok I added dependencies etc. No need for new pull-request ?

@maennchen
Copy link
Contributor

If you push again, the pull request will get updated.

@skol-pro
Copy link
Contributor Author

Ok, already pushed, good.
Indeed, I can see it has been updated.

@maennchen maennchen merged commit 642e4a3 into LuvDaSun:master Mar 17, 2016
@maennchen
Copy link
Contributor

This is the test I wrote:
https://github.com/LuvDaSun/angular-hal/blob/master/test/content-type.spec.js

I had to improve a few things which you see in the latest commit.

Thank you very much.

@skol-pro
Copy link
Contributor Author

Cool, all of this makes sense !
I guess you just forgot to return self; at 1f6ec9b#diff-2963df74eb8dde1731c4892061716901L957 :p

@maennchen
Copy link
Contributor

When angular creates a service, it does it using 'new service()' which will create an instance.
The doesn't have to be returned that way. (but does no harm either...)

@maennchen
Copy link
Contributor

BTW: I just made an improvement to the README.

@skol-pro
Copy link
Contributor Author

Oh really, thought returning self (or this) was necessary.
Perfect regarding the README, nothing can fail anymore !

@maennchen
Copy link
Contributor

function withReturn() { return this;}
function without() {}
withReturn() // withReturn {}
new withReturn() // withReturn {}
without() // undefined
new without() // without {}

@skol-pro
Copy link
Contributor Author

Ah ah :D Ok ok, not sure I got that but that's ok, will keep in mind too.

@skol-pro
Copy link
Contributor Author

You should had the content-type dependency in the README too.
Don't know if user can choose to add the content-type.js stuff or has to do a npm install...

@skol-pro
Copy link
Contributor Author

I mean, for those like who haven't installed angular-hal using npm at least.

@maennchen
Copy link
Contributor

I opened an issue. I'll do it tomorrow.

@skol-pro
Copy link
Contributor Author

Hehe, have a good-night ;-)

@maennchen maennchen self-assigned this Jul 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants