Skip to content

Implement RowsColumnTypeDatabaseTypeName#171

Open
hawkaa wants to merge 1 commit into
alexbrainman:masterfrom
duneanalytics:hakon/column-type-interface
Open

Implement RowsColumnTypeDatabaseTypeName#171
hawkaa wants to merge 1 commit into
alexbrainman:masterfrom
duneanalytics:hakon/column-type-interface

Conversation

@hawkaa

@hawkaa hawkaa commented Feb 1, 2022

Copy link
Copy Markdown

Implement RowsColumnTypeDatabaseTypeName interface in order to return
column types.

https://github.com/golang/go/blob/e22a14b7eb1e4a172d0c20d14a0d2433fdf20e5c/src/database/sql/driver/driver.go#L469-L477

@hawkaa

hawkaa commented Feb 1, 2022

Copy link
Copy Markdown
Author

@vegarsti FYI

@vegarsti

vegarsti commented Feb 1, 2022 via email

Copy link
Copy Markdown
Contributor

@alexbrainman

Copy link
Copy Markdown
Owner

@hawkaa thanks for sending this PR.

But what is the problem that this change solves?

It would be nice if you create an issue first describing the problem, and then we can decide if your change is good solution to your problem.

Thank you.

Alex

@hawkaa

hawkaa commented Mar 7, 2022

Copy link
Copy Markdown
Author

Hi @alexbrainman . I'm terribly sorry that this PR came out of nowhere. I created #174 and I hope that brings clarification.

@alexbrainman alexbrainman left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR.

Please, see my comments.

Alex

Comment thread column.go
@@ -35,6 +35,7 @@ func (l *BufferLen) Bind(h api.SQLHSTMT, idx int, ctype api.SQLSMALLINT, buf []b
// Column provides access to row columns.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add

Fixes #174

to the end of your commit message. So future code readers can determine why we made that change.

Comment thread column.go
// Column provides access to row columns.
type Column interface {
Name() string
DatabaseTypeName() string

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes require a new test in mssql_test.go that demonstrates / verifies new functionality. Otherwise it is impossible for me to judge if new coded works or not.

If you don't have MS SQL Server to test with, see Makefile with instructions on how to run MS SQL Server in Docker on Linux.

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.

5 participants