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

emysql_query and emysql_saver #139

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

Conversation

jacktang
Copy link

@jacktang jacktang commented May 5, 2014

this pull request includes four new modules:

  • emysql_pool.erl: gen_server which maintains pools from OTP application easily
  • emysql_query.erl: which contains find, find_first and find_each functions, and these functions wrap sql queries and record together
  • emysql_saver.erl: which makes saving or updating record in another convenient way.
  • execute_trace.erl: which traces sql execution, now there no flag to enable/disable the function, but it should very important during development.

2414e7c commit is mnesia style transaction you may not interested in.

@jlouis
Copy link
Collaborator

jlouis commented May 5, 2014

Heh, what a massive patch. I think there are a number of things to address before this can go in:

  • There are no tests whatsoever in the new code base. We need tests for the functionality or further patches on the code base will just break stuff.
  • Exporting ?INPUT in the global emysql.hrl is probably going to be problematic. It ought to have a name which is prefixed by emysql in some form.
  • Why the new dependencies?
  • Some of the modules looks as if they could be in a separate "overlay" application rather in the driver itself. emysql_query and emysql_pool for instance. That way, we could split the transaction support so it is the only thing that goes into the driver itself. The remaining modules are probably better kept for themselves.
  • What are the API extensions this patch provides? Blindly adding more functionality to the API is not something I am too keen on doing.

@jacktang
Copy link
Author

jacktang commented May 6, 2014

Hi jlouis,

  • I agreed to adding some high level API extensions and leave low level APIs in driver itself.
  • As to test cases I will add some later
  • ?INPUT it actually return [RecAtom, record_info(?fields, Rec)], I don't think it has something to do with emysql, howerver, it is to name ?EMYSQL_INPUT
  • the dependency espec is for spec testing, I didn't read test cases in emysql project more, but it is used often in our erlang projects, and erl_utils dep provides some utilities for erlang, say type conversion (e.g: any_to_list, any_to_atom) in this project.

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