Skip to content

Conversation

@ferranpujolcamins
Copy link
Contributor

This PR separates the inline snapshot logic into IO and pure code transformation. The code transformation needs the recordings struct, so I made the new function writeInlineSnapshotmonadic. This is useful to write the tests, since we can easily express what code transformations would happen on a real test:

let testExecution = pure
  >>> writeSnapshot1
  >>> writeSnapshot2

I wanted to use swift-prelude, but since it depends on swift-snapshot-testing I couldn't. So I brought some goodies from there and just copied the code here. I hope we can fix this in the future.

@stephencelis
Copy link
Member

Hey @ferranpujolcamins, this is looking really promising! @mbrandonw and I won't have a chance to sit down and really look over this for a bit, though, so sorry for the delay. After a quick skim the thing that sticks out most is bringing in some stuff from Prelude. I think we'll want to minimize that surface area if possible (for example, we defined a minimal Async type rather than bringing in Parallel with all its operators). Is it possible to trim the files you brought in down to bare essentials (that is, what you used in the PR)?

@ferranpujolcamins
Copy link
Contributor Author

Hi @stephencelis

Yes I’ll try to reduce the ported code up to a minimum.

By the way, are you ok using the state monad here? At the end this is not Haskell and a function with inout parameters would also work. If you think it’s best to keep it simple and ditch the state monad please let me know.

@stephencelis
Copy link
Member

Hi @ferranpujolcamins. Sorry for the delay! Let's definitely keep it simple and use inout instead. I think it'll make things easier for future contributors.

@ferranpujolcamins
Copy link
Contributor Author

Done! I used some operators. Now the tests are simple but in case we want to add more complex tests the operators will come handy.

@stephencelis
Copy link
Member

Looking good! Just one last thing and I think we can merge.

I used some operators. Now the tests are simple but in case we want to add more complex tests the operators will come handy.

@mbrandonw and I are definitely fans of operators, but since we're not really using them outside of that one test, maybe we avoid them for now. I'm thinking an inline closure won't be so bad? And should make the code more immediately readable for those unfamiliar with composition operators.

@ferranpujolcamins
Copy link
Contributor Author

Fixed!

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

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

Nice! Thanks so much for making this stuff testable!

@stephencelis
Copy link
Member

Can you resolve the merge conflict? I'll merge as soon as the build is green 🎉

# Conflicts:
#	Sources/SnapshotTesting/AssertInlineSnapshot.swift
@ferranpujolcamins
Copy link
Contributor Author

Done!

@stephencelis stephencelis merged commit 950c0dc into pointfreeco:master Jun 5, 2019
@stephencelis stephencelis mentioned this pull request Sep 25, 2019
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.

2 participants