Skip to content

Make absinthe_plug an optional runtime dependency #14

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

Closed

Conversation

lytedev
Copy link

@lytedev lytedev commented Jun 19, 2019

Per:
https://github.com/opencensus-beam/opencensus_absinthe/pull/13/files#r290670529

This resolves a race condition at compile-time. See upstream PR here:
#13

@codecov-io
Copy link

codecov-io commented Jun 19, 2019

Codecov Report

Merging #14 into master will decrease coverage by 0.74%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #14      +/-   ##
=========================================
- Coverage   28.94%   28.2%   -0.75%     
=========================================
  Files           6       7       +1     
  Lines          38      39       +1     
=========================================
  Hits           11      11              
- Misses         27      28       +1
Impacted Files Coverage Δ
lib/opencensus/absinthe/plug.ex 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01f80bf...7f6d243. Read the comment docs.

@lytedev
Copy link
Author

lytedev commented Jun 19, 2019

I went ahead and lumped in my changes for #15 into this PR. If it makes anybody upset, I can rebase/cherry pick as necessary.

@hauleth hauleth requested a review from garthk June 20, 2019 16:41
@garthk
Copy link
Collaborator

garthk commented Jun 22, 2019

I'd like to keep these separate, yes.

@garthk
Copy link
Collaborator

garthk commented Jun 22, 2019

I've split the race condition from PR #13 into PR #16. @lytedev, please:

@garthk garthk closed this Jun 22, 2019
|> Opencensus.Absinthe.add_phases()
|> Absinthe.Plug.default_pipeline(pipeline_opts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did this work? If so, the Absinthe documentation might need some work. The documentation for Absinthe.Plug/default_pipeline/2 says it takes a map and a keyword list and returns a pipeline, i.e. a list of phases…

Copy link
Author

Choose a reason for hiding this comment

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

Did this work? If so, the Absinthe documentation might need some work. The documentation for Absinthe.Plug/default_pipeline/2 says it takes a map and a keyword list and returns a pipeline, i.e. a list of phases…

Probably not. As you say, this looks like I have lines 31 and 32 in the wrong order. 🤦‍♂

Nice catch!

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.

3 participants