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.

Sign up for GitHub

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

Jump to bottom

[plugin-http] update parseResponseStatus from new specs #642

Closed
OlivierAlbertini opened this issue Dec 21, 2019 · 4 comments · Fixed by #719
Closed

[plugin-http] update parseResponseStatus from new specs #642

OlivierAlbertini opened this issue Dec 21, 2019 · 4 comments · Fixed by #719
Assignees
@OlivierAlbertini
Labels
enhancement New feature or request
Milestone
Alpha v0.4

Comments

@OlivierAlbertini
Copy link
Member

After reading new specs, http plugin need to update parseResponseStatus function...

I believe that switch/case should be removed because it will be too complex (hard to read) to handle new specs.
@OlivierAlbertini OlivierAlbertini added the enhancement New feature or request label Dec 21, 2019
@OlivierAlbertini OlivierAlbertini mentioned this issue Dec 21, 2019
Merged
@dyladan
Copy link
Member

dyladan commented Dec 23, 2019

What would you prefer to have instead of the switch/case? In my opinion it is plenty readable.

@draffensperger
Copy link
Contributor

Would it make sense for this logic to be shared between the web browser XHR and HTTP plugins in some way? Maybe a core-http library of some kind? Not sure if it's worth it since "duplication is better than the wrong abstraction", but wanted to ask the question anyway.

cc/ @obecny

@obecny
Copy link
Member

obecny commented Dec 30, 2019

could we have an enum for that ?

@OlivierAlbertini
Copy link
Member Author

could we have an enum for that ?

We can't have juste an enum, specs requirements need some if/else logics

@OlivierAlbertini OlivierAlbertini self-assigned this Jan 21, 2020
OlivierAlbertini added a commit to VilledeMontreal/opentelemetry-js that referenced this issue Jan 22, 2020
@OlivierAlbertini
feat(plugin-http): sync. specs for statuscode
2a5e3bb 
closes open-telemetry#642

Signed-off-by: Olivier Albertini <[email protected]>
OlivierAlbertini added a commit to VilledeMontreal/opentelemetry-js that referenced this issue Jan 22, 2020
@OlivierAlbertini
feat(plugin-http): sync. specs for statuscode
1410361 
closes open-telemetry#642

Signed-off-by: Olivier Albertini <[email protected]>
@OlivierAlbertini OlivierAlbertini mentioned this issue Jan 22, 2020
Merged
OlivierAlbertini added a commit to VilledeMontreal/opentelemetry-js that referenced this issue Jan 22, 2020
@OlivierAlbertini
feat(plugin-http): sync. specs for statuscode
0639737 
closes open-telemetry#642

Signed-off-by: Olivier Albertini <[email protected]>
@mayurkale22 mayurkale22 modified the milestones: Alpha v0.3.3, Alpha v0.4 Jan 22, 2020
OlivierAlbertini added a commit to VilledeMontreal/opentelemetry-js that referenced this issue Jan 26, 2020
@OlivierAlbertini
feat(plugin-http): sync. specs for statuscode
cd8dec7 
test: fix and add tests
closes open-telemetry#642

Signed-off-by: Olivier Albertini <[email protected]>
dyladan pushed a commit that referenced this issue Jan 28, 2020
@OlivierAlbertini @dyladan
feat(plugin-http): sync. specs for statuscode (#719)
53dda64 
test: fix and add tests
closes #642

Signed-off-by: Olivier Albertini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@OlivierAlbertini OlivierAlbertini

Labels
enhancement New feature or request
Projects
None yet
Milestone
Alpha v0.4
Development

Successfully merging a pull request may close this issue.

feat(plugin-http): sync. specs for statuscode VilledeMontreal/opentelemetry-js
5 participants
@obecny @dyladan @draffensperger @OlivierAlbertini @mayurkale22