Review Feedback

* Remove handler from pollEnvironment to UpdateEnvironment
* Rename handler to errorHandler
* Create a struct FlagsmithErrorHandler so that we can return error and other client response details

Author: ajinkyasurya

client.go

@@ -26,12 +26,12 @@ type Client struct {
 	analyticsProcessor *AnalyticsProcessor
 	defaultFlagHandler func(string) (Flag, error)
 
-	client           *resty.Client
-	ctxLocalEval     context.Context
-	ctxAnalytics     context.Context
-	log              Logger
-	offlineHandler   OfflineHandler
-	pollErrorHandler func(error)
+	client         *resty.Client
+	ctxLocalEval   context.Context
+	ctxAnalytics   context.Context
+	log            Logger
+	offlineHandler OfflineHandler
+	errorHandler   func(handler FlagsmithErrorHandler)
 }
 
 // NewClient creates instance of Client with given configuration.
@@ -245,9 +245,6 @@ func (c *Client) pollEnvironment(ctx context.Context) {
 		err := c.UpdateEnvironment(ctx)
 		if err != nil {
 			c.log.Errorf("Failed to update environment: %v", err)
-			if c.pollErrorHandler != nil {
-			    c.pollErrorHandler(err)
-			}
 		}
 	}
 	update()
@@ -271,10 +268,18 @@ func (c *Client) UpdateEnvironment(ctx context.Context) error {
 		Get(c.config.baseURL + "environment-document/")
 
 	if err != nil {
-		return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)}
+		f := &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: error performing request to Flagsmith API: %s", err)}
+		if c.errorHandler != nil {
+			c.errorHandler(FlagsmithErrorHandler{err, resp.StatusCode(), resp.Status()})
+		}
+		return f
 	}
 	if resp.StatusCode() != 200 {
-		return &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())}
+		f := &FlagsmithAPIError{msg: fmt.Sprintf("flagsmith: unexpected response from Flagsmith API: %s", resp.Status())}
+		if c.errorHandler != nil {
+			c.errorHandler(FlagsmithErrorHandler{err, resp.StatusCode(), resp.Status()})
+		}
+		return f
 	}
 	c.environment.Store(&env)
 	identitiesWithOverrides := make(map[string]identities.IdentityModel)

client_export_test.go

@@ -677,10 +677,12 @@ func TestOfflineHandlerIsUsedWhenRequestFails(t *testing.T) {
 }
 
 func TestPollErrorHandlerIsUsedWhenPollFails(t *testing.T) {
-    // Given
-    ctx := context.Background()
-    expectedErrorMessage := "flagsmith: unexpected response from Flagsmith API: 500 Internal Server Error"
-    var capturedError error
+	// Given
+	ctx := context.Background()
+	// expectedErrorMessage := "flagsmith: unexpected response from Flagsmith API: 500 Internal Server Error"
+	var capturedError error
+	var statusCode int
+	var status string
 
 	server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		rw.WriteHeader(http.StatusInternalServerError)
@@ -691,19 +693,18 @@ func TestPollErrorHandlerIsUsedWhenPollFails(t *testing.T) {
 	client := flagsmith.NewClient(fixtures.EnvironmentAPIKey,
 		flagsmith.WithBaseURL(server.URL+"/api/v1/"),
 		flagsmith.WithEnvironmentRefreshInterval(time.Duration(2)*time.Second),
-		flagsmith.WithPollErrorHandler(func(err error) {
-		    capturedError = err
+		flagsmith.WithErrorHandler(func(handler flagsmith.FlagsmithErrorHandler) {
+			capturedError = handler.Err
+			statusCode = handler.ResponseStatusCode
+			status = handler.ResponseStatus
 		}),
 	)
 
-    // when
-    go func() { flagsmith.PollEnvironment(client, ctx) }()
+	// when
+	_ = client.UpdateEnvironment(ctx)
 
-    // stop method in 2 seconds
-    time.Sleep(1 * time.Second)
-    ctx.Done()
-
-    // Then
-    assert.NotNil(t, capturedError)
-    assert.Contains(t, capturedError.Error(), expectedErrorMessage)
+	// Then
+	assert.Equal(t, capturedError, nil)
+	assert.Equal(t, statusCode, 500)
+	assert.Equal(t, status, "500 Internal Server Error")
 }

client_test.go

@@ -8,6 +8,12 @@ type FlagsmithAPIError struct {
 	msg string
 }
 
+type FlagsmithErrorHandler struct {
+	Err                error
+	ResponseStatusCode int
+	ResponseStatus     string
+}
+
 func (e FlagsmithClientError) Error() string {
 	return e.msg
 }

errors.go

@@ -118,9 +118,9 @@ func WithOfflineMode() Option {
 	}
 }
 
-// WithPollErrorHandler provides a way to handle errors that occur during polling of environment
-func WithPollErrorHandler(handler func(err error)) Option {
-    return func(c *Client) {
-        c.pollErrorHandler = handler
-    }
+// WithErrorHandler provides a way to handle errors that occur during update of an environment
+func WithErrorHandler(handler func(handler FlagsmithErrorHandler)) Option {
+	return func(c *Client) {
+		c.errorHandler = handler
+	}
 }