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

Added sm_reload_databases #773

Merged
merged 5 commits into from Oct 11, 2018

Conversation

@CrazyHackGUT
Copy link
Contributor

CrazyHackGUT commented Feb 25, 2018

Added command sm_reload_databases for refreshing "registered" Databases Configurations cache.

Closes #772

@CrazyHackGUT

This comment has been minimized.

Copy link
Contributor Author

CrazyHackGUT commented Jun 20, 2018

Conflicts has been fixed.

SMCResult ReadSMC_NewSection(const SMCStates *states, const char *name);
SMCResult ReadSMC_KeyValue(const SMCStates *states, const char *key, const char *value);
SMCResult ReadSMC_LeavingSection(const SMCStates *states);
void ReadSMC_ParseEnd(bool halted, bool failed);

This comment has been minimized.

Copy link
@Headline

Headline Jun 20, 2018

Member

Not really sure how this is successfully building, but all of the SMC config parsing is in DBConfBuilder now so I don't think they're necessary.

This comment has been minimized.

Copy link
@CrazyHackGUT

CrazyHackGUT Jun 20, 2018

Author Contributor

This lines suggested me by GitHub. I remove it, ok.

Thanks, @Headline
@KyleSanderson

This comment has been minimized.

Copy link
Member

KyleSanderson commented Jul 12, 2018

I like the change but we need a forward to notify plugins when their dbinfo becomes stale; otherwise it's reload the database conf, and then reload the plugin. It's okay if plugins don't implement the forward; but it's not okay if we don't give them any option to.

@CrazyHackGUT

This comment has been minimized.

Copy link
Contributor Author

CrazyHackGUT commented Jul 15, 2018

You suggest add a forward when DB connection can be obsolete when information updated? Right?

@KyleSanderson

This comment has been minimized.

Copy link
Member

KyleSanderson commented Jul 15, 2018

That's right, but the callback should only be called when the dbinfo changes.

@CrazyHackGUT

This comment has been minimized.

Copy link
Contributor Author

CrazyHackGUT commented Jul 17, 2018

This requires some additional changes. For example, we need save all information about connection when installing him.
Also I'm not sure that I can do this without breaking anything.

But if this really required, i can try implement this "feature".
I think, i need add native for setting function-callback when configuration updated.

@KyleSanderson

This comment has been minimized.

Copy link
Member

KyleSanderson commented Jul 17, 2018

hmm. I could be conflating this and that's entirely okay. To me while this would introduce a nice feature to have without a level change, it would introduce a regression where people would expect their plugins to automatically take on the new configuration.

More than happy to hear some feedback from a peer either confirming my initial comment or saying this is okay as-is.

@CrazyHackGUT

This comment has been minimized.

Copy link
Contributor Author

CrazyHackGUT commented Jul 17, 2018

My original goal when i make this changes - add a simple solution for installing new plugins like SourceBans or VIP System without a level change.

I really didn't think about situation when i have already installed plugin and i edit some values in him configuration. This is a good idea, and we need think about question "How to implement this too most convenient to scripters?". I see three ways:

  1. Property Database.ConfigurationChangeHook (or just Database.ChangeHook?) without getter and native SQL_SetChangeHook.
  2. Natives like Database.AddChangeHook, Database.RemoveChangeHook, SQL_AddChangeHook and SQL_RemoveChangeHook.
  3. Natives like SQL_AddChangeConfigHook and SQL_RemoveChangeConfigHook:
/**
 * Called when a configuration is changed.
 *
 * @param configuration     Configuration name (like "default").
 */
typedef SQL_OnConfigChanged = function void(const char[] configuration);

/**
 * Creates a hook for when configuration is changed.
 *
 * @param configuration     Configuration name (like "default").
 * @param callback          An SQL_OnConfigChanged function pointer.
 * @error                   Invalid callback function.
 */
native void SQL_AddChangeConfigHook(const char[] configuration, SQL_OnConfigChanged callback);

/**
 * Removes a hook for when configuration is changed.
 *
 * @param configuration     Configuration name (like "default").
 * @param callback          An SQL_OnConfigChanged function pointer.
 * @error                   Invalid callback function or no active hook on configuration.
 */
native void SQL_RemoveChangeConfigHook(const char[] configuration, SQL_OnConfigChanged callback);

P.S.: Sorry if something in my post you can not understand. I live in Russia, and it's too difficult for me to compose messages in English.

@KyleSanderson

This comment has been minimized.

Copy link
Member

KyleSanderson commented Jul 17, 2018

too difficult for me to compose messages in English.

Better than most bud 😄

add a simple solution for installing new plugins like SourceBans or VIP System without a level change.

I think we can be a little more clever here. If a plugin asks for a database that doesn't exist we should just do a re-parse then and there instead of relying on a command. If the user hasn't updated their database file, add said dbinfo and reload the plugin.

Make sense? 😸

@asherkin

This comment has been minimized.

Copy link
Member

asherkin commented Aug 11, 2018

I think I'm quite happy to take this as-is, if @KyleSanderson's convictions are not too strong? A forward makes sense, and can always be added later, but this is a reasonable minimal implementation that resolves the original problem.

@KyleSanderson

This comment has been minimized.

Copy link
Member

KyleSanderson commented Aug 11, 2018

We already have another command that does similar; technically neither need to exist and this is to paper over implementation issues (that are easily resolvable for his actual use-case).

Irregardless; I'm not too worked up over it.

@asherkin asherkin merged commit ccfd2ff into alliedmodders:master Oct 11, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.