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

Considering v1.7.1 #1410

Closed
methane opened this issue Apr 11, 2023 · 5 comments
Closed

Considering v1.7.1 #1410

methane opened this issue Apr 11, 2023 · 5 comments

Comments

@methane
Copy link
Member

methane commented Apr 11, 2023

After v1.7.0 was released, #1402 was merged.
Before start merging large/backward incompatible features, how about releasing v1.7.1?

If go, I want to fix #1355 in v1.7.1 too.

@shogo82148 How do you think?

@sjmudd
Copy link
Contributor

sjmudd commented Apr 17, 2023

Good morning @methane.

Thanks for looking at this. If you go ahead, and related to discussions perhaps the documentation should favour the use of https://pkg.go.dev/github.com/go-sql-driver/mysql#Config rather than direct configuration of the DSN directly?

You wrote:

And I don't want to recommend to use DSN. We need to move much documentations from DSN readme to Config object.

I realise this is a time-consuming process and may require multiple changes but at least add a comment immediately after the Usage section in https://github.com/go-sql-driver/mysql, indicating this preference. As things stand it's quite easy to overlook this and for the simplest DSN configuration it seems better/simpler to just create a DSN manually which you indicate you would like to avoid. So a simple change there in bold saying "We recommend usage of use the mysql.Config struct and usage of Config.FormatDSN() to generate the DSN as this both simplifies DSN generation and ensures the created value is correct even in more complex cases" or similar would be more visible and lead people in that direction.

As a single paragraph in the right location it should do most of what you need.

@methane
Copy link
Member Author

methane commented Apr 17, 2023

For simple configuration, using DSN and writing DSN manually is OK.
I recommend to use Config for complex configurations.

And I don't recommend to use Config.FormatDSN() at all. We do not guarantee that FormatDSN() always produce round-tripping, correct DSN.
When using Config object, just pass it to OpenDB().

@sjmudd
Copy link
Contributor

sjmudd commented Apr 17, 2023

oh, apologies. I misunderstood the way you wanted to suggest this to be done, but thought I understood the suggested usage was to use the Config struct and then open the database based on that, rather than generate the DSN manually.

@shogo82148
Copy link
Contributor

Before start merging large/backward incompatible features, how about releasing v1.7.1?

@methane It sounds good.

@methane methane closed this as completed Apr 25, 2023
@dolmen
Copy link
Contributor

dolmen commented Jun 1, 2023

@methane wrote:

And I don't recommend to use Config.FormatDSN() at all. We do not guarantee that FormatDSN() always produce round-tripping, correct DSN.
When using Config object, just pass it to OpenDB().

Unfortunately the documentation for Config.FormatDSN() says:

FormatDSN formats the given Config into a DSN string which can be passed to the driver.

The documentation of Config should link to NewConnector which is the step to use OpenDB.

dolmen added a commit to dolmen/mysql that referenced this issue Jun 2, 2023
Advise to use NewConnector instead of FormatDSN because roundtripping is
known to not work well.
See
go-sql-driver#1410 (comment)
methane pushed a commit that referenced this issue Jun 2, 2023
Advise to use NewConnector instead of FormatDSN because roundtripping is known to not work well.
See #1410 (comment)
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

No branches or pull requests

4 participants