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

database/sql: add String method to IsolationLevel #23632

Closed
AlekSi opened this issue Jan 31, 2018 · 5 comments
Closed

database/sql: add String method to IsolationLevel #23632

AlekSi opened this issue Jan 31, 2018 · 5 comments

Comments

@AlekSi
Copy link
Contributor

@AlekSi AlekSi commented Jan 31, 2018

It would be useful in code like this:

func (c *conn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) {
	level := sql.IsolationLevel(opts.Isolation)
	switch level {
	case sql.LevelDefault:
		// …
	case sql.LevelReadUncommitted:
		// …
	default:
		return nil, fmt.Errorf("isolation level %s is not supported", level)
	}

Without it, all driver authors have to implement the mapping in the driver itself, or not bother with helpful error messages. Examples:

  • mysql – does not work as expected due to string([int]) does not work as author probably thought it does
  • lib/pq – numerical level is returned, not very helpful
  • pgx – numerical level is returned, not very helpful
@AlekSi
Copy link
Contributor Author

@AlekSi AlekSi commented Jan 31, 2018

I could implement it if you are ok with it.

AlekSi added a commit to AlekSi/mysql that referenced this issue Jan 31, 2018
It was "mysql: unsupported isolation level: \a"
due to wrong conversion from int to string.

See also golang/go#23632
@mvdan
Copy link
Member

@mvdan mvdan commented Jan 31, 2018

If you're going to use stringer, use its latest version - it should now use strconv and produce slightly better code.

@odeke-em odeke-em changed the title database/sql: add fmt.Stringer to IsolationLevel proposal: database/sql: add fmt.Stringer to IsolationLevel Jan 31, 2018
@gopherbot gopherbot added this to the Proposal milestone Jan 31, 2018
@gopherbot gopherbot added the Proposal label Jan 31, 2018
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jan 31, 2018

@rsc
Copy link
Contributor

@rsc rsc commented Feb 5, 2018

There aren't that many levels. It's OK to write a String method by hand. But sure, adding a String method is fine.

@rsc rsc changed the title proposal: database/sql: add fmt.Stringer to IsolationLevel database/sql: add String method to IsolationLevel Feb 5, 2018
@rsc rsc modified the milestones: Proposal, Go1.11 Feb 5, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 6, 2018

Change https://golang.org/cl/92235 mentions this issue: database/sql: add String method to IsolationLevel

AlekSi added a commit to AlekSi/go that referenced this issue Feb 20, 2018
Fixes golang#23632

Change-Id: I7197e13df6cf28400a6dd86c110f41129550abb6
@gopherbot gopherbot closed this in ef3ab3f Feb 22, 2018
julienschmidt added a commit to go-sql-driver/mysql that referenced this issue Mar 3, 2018
* Fix error message for unsupported isolation level.

It was "mysql: unsupported isolation level: \a"
due to wrong conversion from int to string.

See also golang/go#23632

* Add myself and sort authors.
@golang golang locked and limited conversation to collaborators Feb 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.