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

Make Witchcraft.Chain.do_notation respect chainer #118

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make Witchcraft.Chain.do_notation respect chainer #118

wants to merge 1 commit into from

Conversation

florius0
Copy link
Contributor

@florius0 florius0 commented Apr 16, 2022 •
edited

Summary

This PR fixes Witchcraft.Chain.do_notation/2.
Previously it ignored second argument assuming any chainer is Witchcraft.Chain.chain/2.

However, there is a call of do_notation/2 across Witchcraft codebase in which chainer other then chain/2 is passed:

witchcraft/lib/witchcraft/monad.ex

Lines 284 to 287 in 6c61c3e

defmacro async(sample, do: input) do
returnized = desugar_return(input, sample)
Witchcraft.Chain.do_notation(returnized, &Witchcraft.Monad.async_bind/2)
end

Test plan (required)

mix test should be enough

Closing issues

Fixes #108

After Merge

  • Does this change invalidate any docs or tutorials? If so ensure the changes needed are either made or recorded
  • Does this change require a release to be made? Is so please create and deploy the release

@florius0
Copy link
Contributor Author

Moreover, the fact mention above can be proved as follows.

Running this snippet before would've returned [links: []], which points that no process were spawn_link'ed to shell process (if the snippet is run from shell).

async [] do
  let a = 1
  let b = 2
  
  return a + b
  
  return Process.info(self, :links)
end

Now, it shows that the process is indeed linked: [links: [#PID<0.238.0>]]

@florius0
Copy link
Contributor Author

@QuinnWilton could you take a look?

@cognivore
Copy link

cognivore commented Mar 3, 2023 •
edited

Wow, thanks for the PR, @florius0 I'll review it shortly and incorporate into doma-engineering fork and provide feedback here + at fission discord. 💙 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Witchcraft.Chain.do_notation/2 intentionally ignores chainer
2 participants
@florius0 @cognivore