Make TimeTruncate functional option #1552

methane · 2024-02-17T14:29:09Z

Description

Config.TimeTruncate just added 2 weeks ago and never released.
This PR make the struct field private and introduce "Functional Options Pattern" instead.

See #1548 for background.

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

Summary by CodeRabbit

Refactor
- Improved handling and configuration of date-time values in database connections.
Documentation
- Updated comments in the configuration structure for better clarity.
New Features
- Introduced a method to apply functional options for database configuration, including time truncation settings.

coderabbitai · 2024-02-17T14:29:15Z

Walkthrough

The changes primarily focus on enhancing the handling and configuration of time truncation within a database connection library. Key modifications include making timeTruncate a private field and introducing a functional option to configure it. This approach streamlines the process of setting time truncation preferences and encapsulates configuration details, improving code readability and maintainability. Modifications span across several files, reflecting a cohesive update to how time values are processed and configured in database queries.

Changes

File(s)	Summary of Changes
`connection.go`, `packets.go`	Updated access to `timeTruncate` from public to private within specific functions, aligning with encapsulation changes.
`dsn.go`, `dsn_test.go`	Enhanced the `Config` struct with comments, privacy adjustments, and a new `SetOptions` method. Introduced a `TimeTruncate` functional option and adjusted test settings.
`result.go`	Minor adjustments in code comment formatting related to database query results.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository from git and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 097fe6e and 5c9a97b.

Files selected for processing (5)

connection.go (1 hunks)
dsn.go (5 hunks)
dsn_test.go (1 hunks)
packets.go (1 hunks)
result.go (1 hunks)

Additional comments: 7

result.go (1)

18-19: The documentation update provides a clearer example of how to access the Result interface, which is beneficial for users of the library. This change improves the readability and usability of the documentation without altering any functionality.
connection.go (1)
254-254: The modification to access the timeTruncate field directly aligns with the PR's objective of encapsulating the TimeTruncate configuration. This change is a good practice in terms of encapsulation and code organization. Ensure that all necessary adjustments have been made throughout the codebase to accommodate this change without introducing regressions.

Verification successful

The findings from the script output indicate that the TimeTruncate field has been successfully encapsulated and is now controlled through a functional option, as intended in the PR objectives. This aligns with best practices for encapsulation and code organization. However, it is recommended to manually review the implementation and application of the TimeTruncate functional option across the codebase to ensure it functions as intended without introducing regressions.
Scripts Executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify that all necessary adjustments for the encapsulation of TimeTruncate have been made.
rg --type go 'TimeTruncate' | grep -v 'timeTruncate'
# Expecting no direct accesses to the old public TimeTruncate field outside of defined methods or functional options.
Length of output: 179
dsn_test.go (1)

79-79: The modification of the TimeTruncate field from a string representation to a time.Duration (time.Hour) is a significant improvement in terms of type safety and clarity. This change ensures that the TimeTruncate configuration is explicitly defined as a duration, reducing the potential for errors related to time parsing and improving the readability of the configuration.

However, it's important to ensure that all parts of the codebase that interact with the TimeTruncate field are updated to accommodate this change. This includes not only the tests but also any part of the application that constructs or modifies Config structs with a TimeTruncate value.

dsn.go (3)

47-61: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [37-75]

The restructuring of the Config struct to include comments categorizing fields into non-boolean, boolean, and private is a good practice for readability and maintainability. Moving pubKey and timeTruncate to private fields aligns with the encapsulation objective. However, ensure that all external accesses to these fields have been appropriately updated to use the new functional options or methods provided.

78-111: The introduction of the Functional Options Pattern through the SetOptions method and the TimeTruncate functional option is a significant improvement. It enhances the flexibility and readability of Config instances configuration. The implementation follows the pattern correctly, allowing for future expansion with more options. Ensure that documentation or examples are provided to guide users on how to use these new configuration methods.

298-299: The modification in the FormatDSN method to handle the private timeTruncate field using a conditional check is correct. This change ensures that the timeTruncate setting is included in the DSN string if it has been set. It's a good practice to encapsulate the logic for formatting DSN strings within the Config struct itself, maintaining separation of concerns.

packets.go (1)

1175-1175: The change from mc.cfg.TimeTruncate to mc.cfg.timeTruncate in the writeExecutePacket function aligns with the PR's objective to encapsulate the TimeTruncate configuration. This modification ensures that the TimeTruncate field is now accessed in a more controlled manner, which is a good practice for encapsulation and maintaining the integrity of the Config struct's fields.

However, it's important to ensure that there are corresponding methods or functions introduced to allow controlled access to this now-private field. This includes setting, getting, or modifying the timeTruncate value in a way that aligns with the intended functionality and usage patterns within the driver.

dsn.go

dsn_test.go

coveralls · 2024-02-17T14:44:19Z

coverage: 81.919% (-0.3%) from 82.17%
when pulling 5b603c9 on methane:timetruncate-fop
into 097fe6e on go-sql-driver:master.

dsn.go

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 5c9a97b and e42533e.

Files selected for processing (1)

dsn.go (5 hunks)

Additional comments: 4

dsn.go (4)

47-61: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [37-75]

The reorganization of the Config struct into non-boolean, boolean, and private fields enhances readability and maintainability. Moving timeTruncate to the private section is a good practice for encapsulation. However, ensure that all existing code that previously accessed timeTruncate directly is updated to use the new TimeTruncate functional option.

84-93: The NewConfig function correctly initializes a Config struct with sensible default values. This is a crucial part of the functional options pattern, ensuring that a Config instance starts in a valid state before any options are applied. Consider documenting the default values for each field in the Config struct, as this can aid users in understanding the baseline configuration.

95-103: The Apply method is well-implemented, iterating over the provided options and applying them to the Config instance. It's important to handle errors from options gracefully, as done here. This method is central to the functional options pattern, allowing for flexible and dynamic configuration. Ensure that error handling is consistent across all options that might be added in the future.

544-546: The error handling in the parseDSNParams function for the timeTruncate parameter is improved based on previous comments, providing a more descriptive error message. This enhancement aids users in diagnosing configuration issues related to invalid timeTruncate values. It's a good practice to provide clear, actionable error messages, especially when parsing user input.

coderabbitai · 2024-02-26T09:55:54Z

dsn.go

+// Functional Options Pattern
+// https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
+type Option func(*Config) error
+
 // NewConfig creates a new Config and sets default values.
 func NewConfig() *Config {
-	return &Config{
+	cfg := &Config{
 		Loc:                  time.UTC,
 		MaxAllowedPacket:     defaultMaxAllowedPacket,
 		Logger:               defaultLogger,
 		AllowNativePasswords: true,
 		CheckConnLiveness:    true,
 	}
+
+	return cfg
+}
+
+func (c *Config) Apply(opts ...Option) error {
+	for _, opt := range opts {
+		err := opt(c)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// TimeTruncate sets the time duration to truncate time.Time values in
+// query parameters.
+func TimeTruncate(d time.Duration) Option {
+	return func(cfg *Config) error {
+		cfg.timeTruncate = d
+		return nil
+	}


The introduction of the Option type and the TimeTruncate functional option is a solid application of the Functional Options Pattern, improving the flexibility and readability of configuration. It's important to ensure that the TimeTruncate option is thoroughly tested, especially for edge cases like negative durations or extremely large values that might not make sense in the context of time truncation.

Would you like me to help with generating tests for the TimeTruncate option?

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e42533e and 86075b6.

Files selected for processing (1)

dsn.go (5 hunks)

Files skipped from review as they are similar to previous changes (1)

dsn.go

dsn.go

arnehormann · 2024-02-26T19:22:42Z

+1 LGTM

arnehormann · 2024-02-26T19:24:26Z

dsn.go

@@ -66,17 +68,48 @@ type Config struct {
 	MultiStatements          bool // Allow multiple statements in one query
 	ParseTime                bool // Parse time values to time.Time
 	RejectReadOnly           bool // Reject read-only connections
+
+	// private fields. new options should be come here


private -> unexpected - let's use Go naming

result.go

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 86075b6 and 5b603c9.

Files selected for processing (1)

dsn.go (5 hunks)

Additional comments: 5

dsn.go (5)

47-61: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [37-75]

The restructuring of the Config struct to include private fields (pubKey and timeTruncate) is a good practice for encapsulation. However, ensure that all existing code that previously accessed timeTruncate directly is updated to use the new functional option approach.

78-112: The introduction of the Option type and the TimeTruncate functional option is a clean implementation of the Functional Options Pattern. This approach enhances the configurability and readability of the Config struct. Just ensure that the documentation for these options is clear enough for users to understand how to use them.

84-93: The NewConfig function correctly initializes a Config object with default values. This is a crucial part of using the Functional Options Pattern, as it provides a base configuration that can be modified with options. Good implementation.

95-104: The Apply method is well-implemented, iterating over the provided options and applying them to the Config object. This method is central to the Functional Options Pattern, allowing for flexible configuration. The choice of the method name Apply is consistent with the discussion in the PR comments.

545-547: The error handling in the parseDSNParams function for the timeTruncate parameter has been improved based on previous comments, providing a more descriptive error message. This change enhances the usability and debuggability of the driver when users provide invalid timeTruncate values in the DSN string.

shogo82148

LGTM

methane added 2 commits February 17, 2024 23:12

dsn: make Config.TimeTrancate private

0c5e07e

make Config.TimeTruncate a functional option

5c9a97b

coderabbitai bot reviewed Feb 17, 2024

View reviewed changes

dsn.go Show resolved Hide resolved

dsn_test.go Show resolved Hide resolved

fixup

f4c212e

shogo82148 reviewed Feb 25, 2024

View reviewed changes

dsn.go Outdated Show resolved Hide resolved

shogo82148 reviewed Feb 25, 2024

View reviewed changes

dsn.go Outdated Show resolved Hide resolved

methane added 2 commits February 26, 2024 18:50

revert NewConfig incompatible change

5f2654f

make Option public, and rename SetOptions to Apply

e42533e

coderabbitai bot reviewed Feb 26, 2024

View reviewed changes

add doc comment

86075b6

coderabbitai bot reviewed Feb 26, 2024

View reviewed changes

methane commented Feb 26, 2024

View reviewed changes

dsn.go Show resolved Hide resolved

arnehormann reviewed Feb 26, 2024

View reviewed changes

result.go Show resolved Hide resolved

arnehormann previously approved these changes Feb 26, 2024

View reviewed changes

private -> unexported

5b603c9

methane dismissed arnehormann’s stale review via 5b603c9 February 27, 2024 01:19

coderabbitai bot reviewed Feb 27, 2024

View reviewed changes

shogo82148 approved these changes Mar 6, 2024

View reviewed changes

methane merged commit 6964272 into go-sql-driver:master Mar 6, 2024
36 checks passed

methane deleted the timetruncate-fop branch March 6, 2024 15:32

methane added this to the v1.8.0 milestone Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TimeTruncate functional option #1552

Make TimeTruncate functional option #1552

methane commented Feb 17, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 17, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

CodeRabbit Discord Community

coderabbitai bot left a comment

coveralls commented Feb 17, 2024 •

edited

Loading

coderabbitai bot left a comment

coderabbitai bot Feb 26, 2024

coderabbitai bot left a comment

arnehormann commented Feb 26, 2024

arnehormann Feb 26, 2024

coderabbitai bot left a comment

shogo82148 left a comment

Make TimeTruncate functional option #1552

Make TimeTruncate functional option #1552

Conversation

methane commented Feb 17, 2024 • edited by coderabbitai bot Loading

Description

Checklist

Summary by CodeRabbit

coderabbitai bot commented Feb 17, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

coveralls commented Feb 17, 2024 • edited Loading

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Feb 26, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

arnehormann commented Feb 26, 2024

arnehormann Feb 26, 2024

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

shogo82148 left a comment

Choose a reason for hiding this comment

methane commented Feb 17, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 17, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

coveralls commented Feb 17, 2024 •

edited

Loading