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

feat($http): provide a config object as an argument to header functions #10622

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@pkozlowski-opensource
Member

pkozlowski-opensource commented Jan 2, 2015

This is a cleaned up / alternative version for #7235

@googlebot googlebot added the cla: yes label Jan 2, 2015

@fundon

This comment has been minimized.

Show comment
Hide comment
@fundon

fundon Jan 5, 2015

+1, Nice!

Please merge!

fundon commented Jan 5, 2015

+1, Nice!

Please merge!

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Jan 9, 2015

Member

@caitp / @petebacondarwin / @lgalfaso (or anyone else!) - would you guys mind reviewing this one - it is pretty minimal

Member

pkozlowski-opensource commented Jan 9, 2015

@caitp / @petebacondarwin / @lgalfaso (or anyone else!) - would you guys mind reviewing this one - it is pretty minimal

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 9, 2015

Contributor

taking a quick gander

Contributor

caitp commented Jan 9, 2015

taking a quick gander

Show outdated Hide outdated test/ng/httpSpec.js
expect(config.bar).toBeUndefined();
});

This comment has been minimized.

@caitp

caitp Jan 9, 2015

Contributor

should we also test the main functionality of this feature? :p

@caitp

caitp Jan 9, 2015

Contributor

should we also test the main functionality of this feature? :p

This comment has been minimized.

@pkozlowski-opensource

pkozlowski-opensource Jan 9, 2015

Member

@caitp it is tested as otherwise a mocked request would fail, no?

@pkozlowski-opensource

pkozlowski-opensource Jan 9, 2015

Member

@caitp it is tested as otherwise a mocked request would fail, no?

This comment has been minimized.

@caitp

caitp Jan 9, 2015

Contributor

let me check ngMock one sec... But in spite of that, I think you should have a different spec for that, because it's not explicitly testing that, it's explicitly testing that the passed in config object is a shallow copy

@caitp

caitp Jan 9, 2015

Contributor

let me check ngMock one sec... But in spite of that, I think you should have a different spec for that, because it's not explicitly testing that, it's explicitly testing that the passed in config object is a shallow copy

This comment has been minimized.

@pkozlowski-opensource

pkozlowski-opensource Jan 9, 2015

Member

Yep, I thought of splitting it into 2 separate specs when I saw your comment. Will create 2 separate specs.

@pkozlowski-opensource

pkozlowski-opensource Jan 9, 2015

Member

Yep, I thought of splitting it into 2 separate specs when I saw your comment. Will create 2 separate specs.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jan 15, 2015

Member

Can you guys triage your own PRs please!

Member

petebacondarwin commented Jan 15, 2015

Can you guys triage your own PRs please!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Jan 17, 2015

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot commented Jan 17, 2015

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no and removed cla: yes labels Jan 17, 2015

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Jan 17, 2015

CLAs look good, thanks!

googlebot commented Jan 17, 2015

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jan 17, 2015

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Jan 17, 2015

Member

I've rebased this one and split tests based on the review comments.
@caitp do you want to have another look at this one?

Member

pkozlowski-opensource commented Jan 17, 2015

I've rebased this one and split tests based on the review comments.
@caitp do you want to have another look at this one?

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 17, 2015

Contributor

I'll take a look on Monday

Contributor

caitp commented Jan 17, 2015

I'll take a look on Monday

@caitp caitp assigned caitp and unassigned pkozlowski-opensource Jan 17, 2015

$httpBackend.flush();
});
it('should not allow modifications to a config object in header functions', function() {

This comment has been minimized.

@caitp

caitp Jan 20, 2015

Contributor

should ignore modifications (but no big deal)

@caitp

caitp Jan 20, 2015

Contributor

should ignore modifications (but no big deal)

This comment has been minimized.

@caitp

caitp Jan 20, 2015

Contributor

this will also only "ignore" changes to root properties of the config object, so changes to child object properties won't be ignored :( but I don't think it's worth worrying about it until someone complains

@caitp

caitp Jan 20, 2015

Contributor

this will also only "ignore" changes to root properties of the config object, so changes to child object properties won't be ignored :( but I don't think it's worth worrying about it until someone complains

This comment has been minimized.

@pkozlowski-opensource

pkozlowski-opensource Jan 20, 2015

Member

yeh... but I agree that we shouldn't worry about it too much right now...

@pkozlowski-opensource

pkozlowski-opensource Jan 20, 2015

Member

yeh... but I agree that we shouldn't worry about it too much right now...

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Jan 20, 2015

Contributor

still lgtm

Contributor

caitp commented Jan 20, 2015

still lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment