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

Removing Fuzz.andThen #183

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jwoudenberg
Collaborator

jwoudenberg commented Jul 1, 2017

Resolves #160 and #161.

This removes Fuzz.andThen making it a breaking change.
It also fixes two points @mgold alluded to before:

  • the Fuzzer type is wrapped in a type instead of being a type alias to prevent people from casing on it. This is not a major change according to elm-package but it is to a user who for some reason is casing on the fuzzer now, so it makes sense to me to bundle the change in here.
  • Move the last two functions in Fuzz.Internal over to Fuzz.

Some things I think should be wrapped up before merging this:

  • Make solutions for users using andThen for building recusive fuzzers or Msg fuzzers presentable, either by publishing them in a library or linking to them as a sort of cookbook.
  • Consider if there's other breaking changes that should be bundled with this change. Personally, I'd like to change Test.Runner.fuzz so it doesn't need a Debug.crash anymore (the last in the codebase).

jwoudenberg added some commits Jul 1, 2017

Do not express Fuzzer as a type alias
It allows user to case on it using the `Ok` and `Err` instructors, which
would make their tests dependant on the `Fuzzer` implementation.

elm-package doesn't consider this a major change, but it is one to users
who were doing this type of pattern matching.
Clean up Fuzz.Internal
It's sole purpose now is containing the hidden internal type of the Fuzzer,
like before.
@jwoudenberg

This comment has been minimized.

Show comment
Hide comment
@jwoudenberg

jwoudenberg Jul 9, 2017

Collaborator

From my perspective this is ready to merge but of course happy to make adjustments!

Collaborator

jwoudenberg commented Jul 9, 2017

From my perspective this is ready to merge but of course happy to make adjustments!

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jul 9, 2017

Member

Love it! Looks good to me!

Member

rtfeldman commented Jul 9, 2017

Love it! Looks good to me!

Show outdated Hide outdated src/Test/Runner.elm
@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Jul 9, 2017

Member

This looks good to me. I'm sorting through upcoming changes and don't want breaking API on master right this moment though.

Member

mgold commented Jul 9, 2017

This looks good to me. I'm sorting through upcoming changes and don't want breaking API on master right this moment though.

@mgold mgold referenced this pull request Jul 9, 2017

Closed

Planning for 0.19 and 5.0.0 #191

6 of 11 tasks complete
@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Jul 9, 2017

Member
Member

rtfeldman commented Jul 9, 2017

@mgold

This comment has been minimized.

Show comment
Hide comment
@mgold

mgold Aug 1, 2017

Member

Tagging major-release-blocker so we don't forget about this. Merge conflicts suck but I don't think it's worth resolving them now is we're still not ready to merge. Merging onto a breaking branch will just give us conflicts later.

Member

mgold commented Aug 1, 2017

Tagging major-release-blocker so we don't forget about this. Merge conflicts suck but I don't think it's worth resolving them now is we're still not ready to merge. Merging onto a breaking branch will just give us conflicts later.

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