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

Made named connections inherit their charset from the default(unless cha... #59

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@esyphelon

esyphelon commented Jan 9, 2015

Changed the way named connections apply their settings to inherit the default connection's charset.

@bigpresh

This comment has been minimized.

Show comment
Hide comment
@bigpresh

bigpresh Jan 19, 2015

Owner

Sorry for the slow reply - this looks good, thanks! However, I'm wondering if it should be more generic, and copy everything from the top level (except the 'settings' key), as it may also be desirable for e.g. dbi_params, handle_class etc to inherit. Need to have a bit more of a think, but I'd welcome your opinions too.

@ambs what do you think also?

Owner

bigpresh commented Jan 19, 2015

Sorry for the slow reply - this looks good, thanks! However, I'm wondering if it should be more generic, and copy everything from the top level (except the 'settings' key), as it may also be desirable for e.g. dbi_params, handle_class etc to inherit. Need to have a bit more of a think, but I'd welcome your opinions too.

@ambs what do you think also?

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs Jan 19, 2015

Collaborator

Indeed, some more inheritance might be good. Probably we can help @esyphelon preparing the list of keys we think should be preserved?

Collaborator

ambs commented Jan 19, 2015

Indeed, some more inheritance might be good. Probably we can help @esyphelon preparing the list of keys we think should be preserved?

@bigpresh

This comment has been minimized.

Show comment
Hide comment
@bigpresh

bigpresh Jan 19, 2015

Owner

On Mon, 19 Jan 2015 02:01:05 -0800
Alberto Simões notifications@github.com wrote:

Indeed, some more inheritance might be good. Probably we can help
@esyphelon preparing the list of keys we think should be preserved?

Off the top of my head, I would say:

  • Take a copy of the top-level plugin config; remove the 'settings' key
    from it;
  • From the named setting's config that has been fetched, copy every key
    specified in it over the top of the copy of the top-level config
    (overwriting any value that was at the top level with what that named
    setting provided)

That way, everything inherits, and future settings will automatically
inherit without having to remember to add them to the list.

Does that seem to make sense? I'm only part way through my first
coffee, so I may have missed something obviously stupid about the
above :)

Owner

bigpresh commented Jan 19, 2015

On Mon, 19 Jan 2015 02:01:05 -0800
Alberto Simões notifications@github.com wrote:

Indeed, some more inheritance might be good. Probably we can help
@esyphelon preparing the list of keys we think should be preserved?

Off the top of my head, I would say:

  • Take a copy of the top-level plugin config; remove the 'settings' key
    from it;
  • From the named setting's config that has been fetched, copy every key
    specified in it over the top of the copy of the top-level config
    (overwriting any value that was at the top level with what that named
    setting provided)

That way, everything inherits, and future settings will automatically
inherit without having to remember to add them to the list.

Does that seem to make sense? I'm only part way through my first
coffee, so I may have missed something obviously stupid about the
above :)

@ambs

This comment has been minimized.

Show comment
Hide comment
@ambs

ambs Jan 19, 2015

Collaborator

It makes sense, but I haven't a coffee yet :-)

Collaborator

ambs commented Jan 19, 2015

It makes sense, but I haven't a coffee yet :-)

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