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

Refactored and prepared erlog for production use #13

Open
wants to merge 265 commits into
base: master
Choose a base branch
from

Conversation

comtihon
Copy link

  • got rid of passing functions everywhere. Maybe it is little bit faster, but code is hard to maintain. Refactored erlog gen_server, made erlog logic more OTP like: now there is one function to call - execute and all work is done through handle_call. Server state (db and others) is kept in #state of gen_server.
  • refactored erlog_int, got rid of defining code (as from my point of view it is not good way), move storage functions from erlog_int to callback interface, made callback interface realisation for ets and dict (dict not working properly for now).
  • created erlog_memory - gen_server, which works with memory (dict, ets, database, and others implementation). There is no need to pass db param as argument everywhere in code. It is stored is #state of erlog gen_server. (But I don't remove passing db in some places, as there are lots of them).
  • moved all modules to folders
  • moved functions from huge (500+ line of codes) modules to smaller one
  • erlog_int -> erlog_memory, erlog_core

@rvirding
Copy link
Owner

There is a lot of stuff here to go through! I will pull it locally and check it out. This may take some time as I am summer holidays and my programming has become more relaxed. Also I have to prepare a talk + demo for OSCON so please don't expect a speedy response, but I will get back to you on this.

I noticed you have worked on the interface. I have just written up some independent thoughts on this to the erlog mailing list. It is a Google group at:

https://groups.google.com/forum/?fromgroups=#!forum/erlog

Please join if you wish. Traffic is low and I am trying to get more potential users and have it as a place for discussions.

@comtihon
Copy link
Author

I joined and posted my answer.
May be you should review my code, and then continue working on project,
as I think some of your thoughts you may find there.

@zkessin
Copy link
Contributor

zkessin commented Jun 26, 2014

Question, why does execute return a binary take this code:

fail_test() ->
    {ok, PID}   = erlog:start_link(),
    ?assertEqual(fail,erlog:execute(PID, "fail.")),
    true.

erlog:execute/2 returns the binary <<"No">>, might it make more sense to return it as an atom 'no' or better yet 'false'? It definitely seems more like other erlang functions to me

@comtihon
Copy link
Author

It doesn't matter, I suppose, but if you prefer - it can be true/false.
There is no direct standart for return in prolog. In some prologs they
return No, in some - false.

@zkessin
Copy link
Contributor

zkessin commented Jun 28, 2014

Well having it be bool() makes it work a lot better with many erlang things.

@rvirding
Copy link
Owner

I don't think it should return a boolean or a binary. I think proving goal should return the variable bindings if it succeeds so a boolean wouldn't fit, something like {true,Bindings} is just bad. I know we have a a few times in the libraries but that doesn't mean we should imitate it. And is shouldn't be false | NotBoolean.

That is why I chose {succeed,Bindings} | fail.

@zkessin
Copy link
Contributor

zkessin commented Jun 29, 2014

What @rvirding said, the return state should be what it was {succeed,[binding()]}|fail, also take a look at the quickcheck properties I created and lets see if we can make those work.

@comtihon
Copy link
Author

comtihon commented Jul 1, 2014

So, what are the plans for merging?
What's the verdict?

@rvirding
Copy link
Owner

rvirding commented Jul 2, 2014

As your PR does some restructuring and renaming I would first like a discussion on how we want to structure the erlog distribution. I will start one on the mailing-list

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