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

Change - Documentation Update #1380

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

nidhi752
Copy link

@nidhi752 nidhi752 commented Jan 9, 2025

Pull Request Template

Description:

Checklist:

  • I have formatted my code using goimport and golangci-lint.
  • All new code is covered by unit tests.
  • This PR does not decrease the overall code coverage.
  • I have reviewed the code comments and documentation for clarity.

Thank you for your contribution!

@nidhi752 nidhi752 changed the title Change Change - Documentation Update Jan 9, 2025
@Umang01-hash
Copy link
Contributor

@nidhi752 Still there are some linter errors in your PR. kindly have a look at the workflow run : https://github.com/gofr-dev/gofr/actions/runs/12684562987?pr=1380 and resolve the linters
Screenshot 2025-01-09 at 2 39 51 PM

func New(config Config) *Client {
return &Client{config: config}
}

// UseLogger sets the logger for the Clickhouse client.
// The logger must implement the Logger interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use godoc link

Suggested change
// The logger must implement the Logger interface.
// The logger must implement the [Logger] interface.

Also the must is a bit strong as you can pass something that doesn't, it would be ignored

func (c *Client) UseLogger(logger interface{}) {
if l, ok := logger.(Logger); ok {
c.logger = l
}
}

// UseMetrics sets the metrics for the Clickhouse client.
// The metrics must implement the Metrics interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The metrics must implement the Metrics interface.
// The metrics must implement the [Metrics] interface.

And same remark about "must"

)


// Config defines the configuration needed to connect to a Clickhouse database.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Config defines the configuration needed to connect to a Clickhouse database.
// Config defines the configuration needed to connect to a ClickHouse database.

}

// Client represents the Clickhouse client, encapsulating connection details, configuration, logging, metrics, and tracing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Client represents the Clickhouse client, encapsulating connection details, configuration, logging, metrics, and tracing.
// Client represents the ClickHouse client, encapsulating connection details, configuration, logging, metrics, and tracing.

// client.UseMetrics(Metrics())
//
// client.Connect()
// New initializes a new Clickhouse client with the provided configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// New initializes a new Clickhouse client with the provided configuration.
// New initializes a new ClickHouse client with the provided configuration.

func (c *Client) UseMetrics(metrics interface{}) {
if m, ok := metrics.(Metrics); ok {
c.metrics = m
}
}

// UseTracer sets the tracer for Clickhouse client.
// UseTracer sets the OpenTelemetry tracer for the Clickhouse client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// UseTracer sets the OpenTelemetry tracer for the Clickhouse client.
// UseTracer sets the OpenTelemetry tracer for the ClickHouse client.

func (c *Client) UseTracer(tracer any) {
if t, ok := tracer.(trace.Tracer); ok {
c.tracer = t
}
}

// Connect establishes a connection to Clickhouse and registers metrics using the provided configuration when the client was Created.
// Connect establishes a connection to the Clickhouse database using the client's configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Connect establishes a connection to the Clickhouse database using the client's configuration.
// Connect establishes a connection to the ClickHouse database using the client's configuration.

@ccoVeille ccoVeille mentioned this pull request Jan 9, 2025
4 tasks
@Umang01-hash
Copy link
Contributor

@nidhi752 Please address the review comments given in the PR.

@nidhi752
Copy link
Author

@Umang01-hash
does this still need any changes sir?

@Umang01-hash
Copy link
Contributor

@Umang01-hash does this still need any changes sir?

Yes @nidhi752 @ccoVeille has requested a few please check

@Umang01-hash
Copy link
Contributor

@nidhi752 Requesting you to please work on the review comments else we will need to close the PR due to inactivity.

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.

3 participants