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

client: Make example testable #987

Merged
merged 4 commits into from
Feb 21, 2025

Conversation

dveeden
Copy link
Collaborator

@dveeden dveeden commented Feb 9, 2025

This removes the client example from the README and puts it in its own file.

The benefits of this:

The drawbacks of this:

  • It isn't in the README anymore
  • This might be a bit weird for server examples that need the MySQL client. But there we could just have no output.

Issues found with the example while doing this:

  1. Missing import for github.com/go-mysql-org/go-mysql/mysql (it uses mysql.FieldValueTypeFloat)
  2. The example uses a table named table, which is confusing.
  3. The example doesn't create the table
  4. The defer r.Close() should probably be moved higher up
  5. The r.GetInt() and r.GetIntByName() probably need more explanation
  6. The test for the float type is never true
  7. The values are never printed to standard out
  8. The code doesn't run as it has an import, but the rest of the code isn't in a function as expected.
  9. The r variable is created twice with :=
  10. Not every result is closed
  11. Error handling isn't done (Maybe that's ok or even desired for examples?)

Setting this as draft as I'm not sure if this is the right thing to do or not.

Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

rest lgtm

@dveeden
Copy link
Collaborator Author

dveeden commented Feb 10, 2025

This is how this looks with godoc:
image

Top part ↑

Bottom part ↓

image

So this only renders the contents of the Example function, not the context.

@dveeden
Copy link
Collaborator Author

dveeden commented Feb 10, 2025

This is after fixing the way I run godoc: (now the import is correct)
image

@lance6716
Copy link
Collaborator

Oh you mean the the purpose of Example is to improve godoc of this package? They are also runnable by go test -v ./client -run Example so I think we should also consider the friendly instruction when running it.

@lance6716
Copy link
Collaborator

Hi, do you need some help on this PR?

@dveeden
Copy link
Collaborator Author

dveeden commented Feb 16, 2025

Hi, do you need some help on this PR?

Still not 100% sure what the best way is. But I'm getting more sure that testable examples are the right choice.

dveeden and others added 2 commits February 17, 2025 07:20
@lance6716
Copy link
Collaborator

I have added my commit, PTAL @dveeden

@lance6716 lance6716 marked this pull request as ready for review February 19, 2025 01:26
@lance6716 lance6716 merged commit 8aa9aa6 into go-mysql-org:master Feb 21, 2025
15 checks passed
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.

None yet

2 participants