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.

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

Refactor meck into smaller functional modules #82

Merged
merged 11 commits into from
Nov 4, 2012
Merged

Refactor meck into smaller functional modules #82

merged 11 commits into from
Nov 4, 2012

Conversation

horkhe
Copy link
Contributor

@horkhe horkhe commented Oct 28, 2012

No description provided.

@horkhe
Copy link
Contributor Author

horkhe commented Oct 24, 2012

@eproxus First draft is ready for you to check if I am going in the right direction. It is located under feature/mock_proc branch of my fork. There meck was split into four modules:

  • meck - facade module that provides the library API;
  • meck_proc - gen_server that maintains mocked module state. (one instance per module);
  • meck_code_gen - contains code that generates mocked modules code and contains pieces that are called in the context of the generated modules;
  • meck_history - functions to dig info from call history.
  • meck_util - functions that are used throughout the code but do not belong anywhere in particular.

Type specs were mostly put in place.

All unit tests pass.

Dialyzer issues 3 reasonable warnings see ./dialyzer.ignore-warnings for details.

It really make no sence to look at diff for the changes are massive.

Hit me :)

beapirate and others added 7 commits October 24, 2012 21:53
- get rid of the process dictionary;
- make tests run on any OS;
Original meck.erl was split into:
- meck - facade for entire library;
- meck_code_gen - code generation of mocked modules;
- meck_proc - satelite gen_server for every mocked module to
  maintain state (list of mocked functions, call history);
- meck_history - module that contains history digging functions;
- meck_util - misc stuff.

Function type specs were mostly put in place.

All unit tests pass.

Dialyzer issues only 3 sane warnings see
./dialyser.ingnore-warnings for details.
@horkhe
Copy link
Contributor Author

horkhe commented Oct 26, 2012

I have just force pushed changes to the same commit. Want be doing this anymore. All further changes will go as individual commits.

Besides the specified file module split up I fell erge to rename meck_mod to meck_code_util.

Anxiously waiting for you opinion on all these mess...

- meck_expect module was introduced to contain
  expectation construction and matching logic;
- history related types were moved to meck_history;
@horkhe
Copy link
Contributor Author

horkhe commented Oct 26, 2012

Another big chunk of functionality was isolated in a module - meck_expect. As the name suggests it contains functions to create expectations and match them. It accumulated functionality formerly spread around meck, meck_proc, and meck_util.

None of the modules is longer then 500 lines of code now. And most of them are below 200. They all have rather slim API and most of the functions are private.

@eproxus
Copy link
Owner

eproxus commented Oct 28, 2012

Comments:

  • I think renaming meck_mod to meck_code would be enough

  • There are a few circular dependencies that I'd like to see go away: Imgur
    These are:

    • meck_code_genmeck
    • meck_code_genmeck_proc

    It seems that these dependencies are because the module meck_code_gen contains a lot of run time specific code. This should not be the case.

  • I'd like to avoid naming functions do_... because it is redundant. One example is do_get_ret_specs for the gen server action get_ret_specs. This can be just get_ret_specs(...). Maybe this was there before?

All in all, it looks good and I'd like to merge this as soon as the changes above are integrated.

@horkhe
Copy link
Contributor Author

horkhe commented Oct 28, 2012

I will address these comments as soon as possible. How did yo draw this nice picture of deps by the way?

@eproxus
Copy link
Owner

eproxus commented Oct 28, 2012

My other project: https://github.com/eproxus/grapherl 😃

- meck_mod was renamed to meck_code;
- cyclic dependency between meck and meck_code_gen
  was removed.
@horkhe
Copy link
Contributor Author

horkhe commented Oct 28, 2012

Already fixed:

  • meck_mod was renamed to meck_code;
  • cyclic dependency between meck and meck_code_gen.

However I do not know how to resolve the cycle between meck_proc and meck_code_gen. meck_proc calls meck_code_gen:to_forms/2 only. And meck_code_gen calls meck_proc:get_ret_spec/3, meck_proc:add_history/5, and meck_proc:invalidate/1. Everything seems reasonable to me. Any concrete suggestions?

There are only two do_xxx functions and they both are implementations on a gen server API call. the reason for do_ prefix is that arity of internal functions is the same as of their API counterparts. Originally get_ret_spec was called get_expect and its implementation counterpart has the same name but different arity. I renamed it to better reflect the semantic of returned value. The other do_ function is do_delete_expect, originally API was delete and implementation was delete_expect. I renamed API yet again to make intention clearer. Please advise if this explanation seems reasonable to you and if not then please provide you suggestion.

Thanks a lot Adam for your comments. Great graph tool by the way.

@horkhe
Copy link
Contributor Author

horkhe commented Oct 28, 2012

Since you you want to get this integrated sooner rather then latter I attached code to this issue.

@horkhe
Copy link
Contributor Author

horkhe commented Nov 1, 2012

Travis CI status shows that the build against this pull request failed but it looks like a Travis issue itself.

@eproxus
Copy link
Owner

eproxus commented Nov 4, 2012

Let's leave the dependencies for later then. The do_...-naming sounds reasonable, so let's go with that for now.

eproxus added a commit that referenced this pull request Nov 4, 2012
Refactor meck into smaller functional modules
@eproxus eproxus merged commit 68b3814 into eproxus:develop Nov 4, 2012
@horkhe horkhe mentioned this pull request Nov 4, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants