Implement SetQueryParams in the Provider Interface #18

yendelevium · 2024-10-20T06:49:58Z

Changed the function signature of the FetchData method in the Provides interface
Added the SetQueryParams method to the providers aligned to the concerns raised in feature: Providers must implement Query Parameters #13

Resolves #13
References #5

- Changed the function signature of the FetchData method in the Provides interface - Added the SetQueryParams method to the providers aligned to the concerns raised in tinkershack#13 Resolves tinkershack#13 References tinkershack#5

adofm · 2024-10-20T07:39:04Z

providers/meteoblue.go

-// Maybe add a MeteoBlueQueryParams like OpenMeteoQueryParams to have some standard for all providers?
+// Set the QueryParams for the request
+func (p *MeteoBlueProvider) SetQueryParams(coords *plumber.Coordinates) map[string]string {
+	// Get the config to accesss the API Key


we can add a check to see whether the coords aren't nil ?

I thought we could do this when we define the endpoints, before calling fetchData itself. If it's nil, we don't make a request to the provider and write the err response

raamsri

This patch makes FetchData() receive and set Location dynamically. That's good.
The idea is to reduce the movement/redundant passing of QP, however this patch hasn't addressed it yet; since it returns the QP data, it must then be passed around. Please consider the following changes?

a. OpenMeteoProvider struct must have QueryParams field to hold the QP values that is of type map[string]string

b. SetQueryParams(*plumber.Coordinates) must set the field OpenMeteoProvider.QueryParams with appropriate values. Since it's a method on OpenMeteoProvider that sets its own field, it doesn't have to return anything. In this patch, it's instead returning the query params rather than setting it.

Please correct if I have misunderstand something.

c. NewOpenMeteoProvider() and NewMeteoBlueProvider() must appropriately call their respective SetQueryParams() method to set the QP before returning a new struct. (A default location, or nil, may be set here.) It would be convenient to also make the appropriate p.client.SetQueryParams() to resty with this QP before returning the struct, so that it sticks and only the location cords may have to passed while fetching data.

d. FetchData() need not change location data of QueryParams field of the MeteoProvider struct as it's a transient call. This side effect is misleading, and not necessary. It simply has to set the passed location co-ords via p.client.SetQueryParams() as the other QP is already set earlier while creating a new provider. That way, only the location co-ords has to be set freshly, and that's the intention as well.

yendelevium · 2024-10-21T05:53:14Z

This patch makes FetchData() receive and set Location dynamically. That's good.

The idea is to reduce the movement/redundant passing of QP, however this patch hasn't addressed it yet; since it returns the QP data, it must then be passed around. Please consider the following changes?

a. OpenMeteoProvider struct must have QueryParams field to hold the QP values that is of type map[string]string

b. SetQueryParams(*plumber.Coordinates) must set the field OpenMeteoProvider.QueryParams with appropriate values. Since it's a method on OpenMeteoProvider that sets its own field, it doesn't have to return anything. In this patch, it's instead returning the query params rather than setting it.

Please correct if I have misunderstand something.

c. NewOpenMeteoProvider() and NewMeteoBlueProvider() must appropriately call their respective SetQueryParams() method to set the QP before returning a new struct. (A default location, or nil, may be set here.) It would be convenient to also make the appropriate p.client.SetQueryParams() to resty with this QP before returning the struct, so that it sticks and only the location cords may have to passed while fetching data.

d. FetchData() need not change location data of QueryParams field of the MeteoProvider struct as it's a transient call. This side effect is misleading, and not necessary. It simply has to set the passed location co-ords via p.client.SetQueryParams() as the other QP is already set earlier while creating a new provider. That way, only the location co-ords has to be set freshly, and that's the intention as well.

Makes sense, I will implement them.

- Add queryParams field on the providers - Update SetQueryParams method to set the queryParams field on the provider - Update NewOpenMeteoProvider() and NewMeteoBlueProvider() to set default QPs on the client - Update FetchData() to only set the coords, and not the entire QPs

yendelevium · 2024-10-21T06:46:08Z

Have implemented the changes @adofm @raamsri , Please review

raamsri · 2024-10-22T10:59:43Z

providers/meteoblue.go

+func (p *MeteoBlueProvider) FetchData(coords *plumber.Coordinates) (*plumber.BaseData, error) {
+	resp, err := p.client.
+		SetQueryParams(map[string]string{
+			"latitude":  fmt.Sprintf("%f", coords.Latitude),


raamsri

LGTM. ~~Just the typo of the query fields "lat" and "long" needs fixing.~~

raamsri · 2024-10-22T11:00:22Z

providers/open_meteo.go

+func (p *OpenMeteoProvider) FetchData(coords *plumber.Coordinates) (*plumber.BaseData, error) {
+	resp, err := p.client.
+		SetQueryParams(map[string]string{
+			"latitude":  fmt.Sprintf("%f", coords.Latitude),


"lat" and "long"

Please ignore. What you have put here is correct.

raamsri · 2024-10-22T11:10:49Z

LGTM. ~~Just the typo of the query fields "lat" and "long" needs fixing.~~

Open Meteo uses "latitude" and "longitude" while Meteoblue uses "lat" and "lon"

raamsri

There's a mix up between the use of "lat" and "lon" for Meteo Blue, in SetQueryParams() call within FetchData().

raamsri · 2024-10-22T11:18:58Z

providers/meteoblue.go

+	resp, err := p.client.
+		SetQueryParams(map[string]string{
+			"latitude":  fmt.Sprintf("%f", coords.Latitude),
+			"longitude": fmt.Sprintf("%f", coords.Longitude),


Open-Meteo uses "latitude" and "longitude" while Meteoblue uses "lat" and "lon".

This forms the URL as "...&lat=0.000000&latitude=10.901838&lon=0.000000&longitude=76.899845&tz=GMT"

Hmm, then how about we don't set the latitude and longitude / lat - long while setting the default query params and directly set them for the first time in the FetchData() call?

I reckon that should be okay.

… of MB

Implement SetQueryParams in the Provider Interface

85aec3c

- Changed the function signature of the FetchData method in the Provides interface - Added the SetQueryParams method to the providers aligned to the concerns raised in tinkershack#13 Resolves tinkershack#13 References tinkershack#5

yendelevium requested review from raamsri and adofm October 20, 2024 06:49

adofm reviewed Oct 20, 2024

View reviewed changes

raamsri requested changes Oct 20, 2024

View reviewed changes

raamsri reviewed Oct 22, 2024

View reviewed changes

raamsri approved these changes Oct 22, 2024

View reviewed changes

raamsri requested changes Oct 22, 2024

View reviewed changes

Address the latitude-longitude/lat-lon discrepancy in the queryParans…

b912216

… of MB

raamsri approved these changes Oct 22, 2024

View reviewed changes

raamsri merged commit 67ac69d into tinkershack:main Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SetQueryParams in the Provider Interface #18

Implement SetQueryParams in the Provider Interface #18

yendelevium commented Oct 20, 2024

adofm Oct 20, 2024

yendelevium Oct 21, 2024 •

edited

Loading

raamsri left a comment •

edited

Loading

yendelevium commented Oct 21, 2024

yendelevium commented Oct 21, 2024

raamsri Oct 22, 2024

raamsri left a comment •

edited

Loading

raamsri Oct 22, 2024

raamsri Oct 22, 2024

raamsri commented Oct 22, 2024 •

edited

Loading

raamsri left a comment •

edited

Loading

raamsri Oct 22, 2024

yendelevium Oct 22, 2024

raamsri Oct 22, 2024

yendelevium Oct 22, 2024

Implement SetQueryParams in the Provider Interface #18

Implement SetQueryParams in the Provider Interface #18

Conversation

yendelevium commented Oct 20, 2024

Choose a reason for hiding this comment

yendelevium Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

raamsri left a comment • edited Loading

Choose a reason for hiding this comment

yendelevium commented Oct 21, 2024

yendelevium commented Oct 21, 2024

Choose a reason for hiding this comment

raamsri left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raamsri commented Oct 22, 2024 • edited Loading

raamsri left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yendelevium Oct 21, 2024 •

edited

Loading

raamsri left a comment •

edited

Loading

raamsri left a comment •

edited

Loading

raamsri commented Oct 22, 2024 •

edited

Loading

raamsri left a comment •

edited

Loading