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

Add default connection attributes #1825

Merged
merged 5 commits into from
Feb 24, 2023
Merged

Conversation

Daemonxiao
Copy link
Contributor

MySQL connectors have some Available Connection Attributes.

I add the following built-in connection attributes to Node-MySQL-2.

{
  '_client_name': 'Node-MySQL-2',
  '_client_version': version
}

These attributes will help users distinguish which driver the connection comes from.

@sidorares
Copy link
Owner

LGTM, thanks for adding this @Daemonxiao

could you link mysql docs in a comment and add few words explaining what/why these attributes are ( and that names are not something we made up and are common agreed )

Also can you rewrite a commit message to follow https://www.conventionalcommits.org/en/v1.0.0/

( also - might need to rebase once again a bit later, currently PlanetScale check is failing for some reason. I'll try to fix or disable that today )

@Daemonxiao
Copy link
Contributor Author

@sidorares Thanks for your review. I have rewritten the commit message. Does it meet the requirements? Please feel free to tell me if you have any advice.

@sidorares
Copy link
Owner

sidorares commented Feb 2, 2023

Does it meet the requirements? Please feel free to tell me if you have any advice.

Yep, perfect. release-please is using commit messages to automatically populate feature / release / chore section of the release post, and also only prepares a release if there a new "releasable items" since the last one.

@sidorares
Copy link
Owner

Maybe it's better to move the change to connection_config, where other default connection parameters are
Also maybe worth leaving a way to override attributes if they are passed, for example here

this.connectAttributes = options.connectAttributes;

use line like

const defaultConnectAttributes =  {
  _client_name: 'Node-MySQL-2',
  _client_version: version
};
this.connectAttributes = { ...defaultConnectAttributes, ...(options.connectAttributes || {}));

@Daemonxiao
Copy link
Contributor Author

Daemonxiao commented Feb 7, 2023

Maybe it's better to move the change to connection_config, where other default connection parameters are
Also maybe worth leaving a way to override attributes if they are passed, for example here

@sidorares Thank you very much. This really excellent advice. I have applied it in nearly commit.

@sidorares
Copy link
Owner

@Daemonxiao some unit tests are failing, could you try running them locally?

@Daemonxiao
Copy link
Contributor Author

some unit tests are failing, could you try running them locally?

@sidorares Done.

@Daemonxiao
Copy link
Contributor Author

@sidorares Is this pr ready to be merged? Or is there anything else I can do?

@sidorares sidorares merged commit d06dd59 into sidorares:master Feb 24, 2023
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.

2 participants