refactored config and dynamic vars to remove global options atom

[k13gomez]

options can now be obtained calling the get-options function, defaults can be defined adding defmethod entries to multimethod get-defaults; see issue #304.

[k13gomez]

You can take a look but I'll have to figure out how to merge this with all the whitespace changes that were just commited...turned into a mess.

[k13gomez]

I rebased the PR branch due to previous white-space changes, there are no conflicts now

[immoh]

All options should only exist in db specs. We let user override the default options when creating the db spec but otherwise global default shouldn't exist separately. I think this also means we can get rid of korma.config completely and move the stuff we need to korma.db

We are already setting db specific default options in helper functions in korma.db, in my view this should be enough and no additional mechanism (e.g. subprotocol based multimethod) should be needed.

I am not sure if korma.dynamic is good idea, is it somehow needed because of ns dependencies?

[k13gomez]

I'll play around to see if we can get rid of korma.config and remove default options completely, it is indeed tricky because of korma.sql.engine dependencies as you and @ls4f previously noted.

korma.dynamic is indeed necessary because of ns dependencies, I'll see if I can avoid it but will first focus on the issue above.

[k13gomez]

Take a look at this PR again, I successfully got rid of korma.config, and didn't need korma.dynamic; options are now obtained from the current-db for all engine/core code.

[immoh]

Sorry for taking so long. It looks better but still more complicated than what I would like it to be.

Macro db/bind-db makes the application logic really difficult to follow since it's partly duplicating what with-db and do-query are doing but with a slightly different logic. Is there be anyway to get rid of this? Is it only needed for relations? I don't think korma.core should set value of db/current-db in any situation, I think the original idea was that we assoc the db of the main query to child entity (if not already set) and let do-query pick it from there. Are there cases where this doesn't work?

Is there any chance to simplify query handling so that we wouldn't have to look for :db in both query and entity of query?

Other minor observations:

• I don't think the changes in ->naming add any value for the users, we should stick to the old simpler version and accept only maps as argument
• global-config test should be deleted, it was there only to ensure that the regression was fixed in latest release
• please make a separate commit for .gitignore changes

[k13gomez]

Bind-db only binds the current-db if it is not already bound (whereas with-db always rebinds), also bind-db does not create/open a database connection. This is mainly used by the sql engine, relationship post-queries, and when adding joins. This should be safe since bind-db does not replace an already bound current-db, but let me know if you can think of a better approach (see below).

• Taking your suggestions into consideration I replaced calls from core to bind-db with calls to bind-query (which was called in other locations) which still works correctly.
• I also simplified the query handling to not look in both query and entity of query and it works fine.
• Finally, I incorporated your minor observations/suggestions and will do a separate PR for .gitignore changes.

[immoh]

Ok, looks good. I will need to dig a little deeper into the code but most likely I won't have time for that before next week.

[immoh]

Sorry for taking so long.

The reason why I don’t like bind-db is that we don’t always have db bound to current-db, and bind-db is trying to patch it by binding a db, possibly wrong one. This solution is equivalent to previous bound-options solution but in my opinion it’s a bit more confusing as it is using same dynamic var as actual connection handling.

In order to use options from current-db for sql generation we should only bind it in one place (maybe in the beginning of korma.core/exec). I played a bit with this idea and found out that many identifier names (with delimiters) are already generated before exec, e.g. where condition column names, join column names. This should be fixed so that no identifiers are generated before we are in exec. This probably requires quite a lot of work to get it right.

I made a commit on top of your branch that reverts back to using bound-options: https://github.com/korma/Korma/tree/k13gomez-remove_global_options. I think something similar to this would be acceptable to merge as it shouldn’t make codebase any worse but we need to really fix sql generation so that it happens as late as possible.

At least one more thing to address is that with this solution you cannot use sql-only to generate sql if you don’t have default db set. I think we should allow this by using the default options.

[k13gomez]

I encountered the same problem with identifiers being generated before exec, and that's why I took this route for this PR. It would indeed require a lot more work to get right an approach where the identifiers are evaluated only at exec time, it would be a good enhancement.

I pulled your change to revert back to using bound-options into the PR branch; I think not being able to generate sql-only is probably an edge case we can ignore for now, the only way we could achieve such behavior would be to go back to having some kind of global default options. Let me know if you still want to continue to work this aspect into the PR.

[immoh]

Yeah, it's an edge case and can be easily worked around by defining dummy database (e.g. (defdb db (mysql {}))) so let's leave it out for now.

Can you please squash the commits?

[k13gomez]

Great, squashing done.

[k13gomez]

...and resolved conflicts with master branch

[immoh]

Thanks!