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

Fix: change the logic of equality checking for Widget IDs #17

Merged
merged 2 commits into from Jan 11, 2021
Merged

Fix: change the logic of equality checking for Widget IDs #17

merged 2 commits into from Jan 11, 2021

Conversation

ycd
Copy link
Contributor

@ycd ycd commented Dec 10, 2020

Hey Max, thanks for this great package.

Today I encountered a bug (I assume it is). When using multiple ComparisonItem, checking the equality of widgets is failing.

Reproducing the bug:

I created an ExploreWidget using Explore function.

compare, err := gogtrends.Explore(ctx, &gogtrends.ExploreRequest{
	ComparisonItems: []*gogtrends.ComparisonItem{
		{
			Keyword: keywords[0],
			Geo:     location,
			Time:    date,
		},
		{
			Keyword: keywords[1],
			Geo:     location,
			Time:    date,
		},
	},
}, "EN")

Everything works fine here, this is the output of the ExploreWidget when I iterate it.

&{APP6_UEAAAAAX9OjpNJhEVoj3prChllfkoRP9ExHVy2s fe_line_chart Interest over time TIMESERIES 0xc0004ea240}
&{APP6_UEAAAAAX9OjpHPeKZIoXpXma9ciAnznpU3g4rsb fe_multi_heat_map Compared breakdown by subregion GEO_MAP 0xc0004ea5a0}
&{ fe_text  TITLE_0 <nil>}
&{APP6_UEAAAAAX9OjpIMIbe5xddzcCMLkyJo-QiZ9jb-_ fe_geo_chart_explore Interest by subregion GEO_MAP_0 0xc0004ea900}
&{APP6_UEAAAAAX9OjpG-7Vgq0uturH_LIpbhmmlxDMXuw fe_related_searches Related queries RELATED_QUERIES_0 0xc0004eab40}
&{ fe_text  TITLE_1 <nil>}
&{APP6_UEAAAAAX9OjpBteqtm7Twc9Wkqk3ubMmg-a1XdV fe_geo_chart_explore Interest by subregion GEO_MAP_1 0xc0004eaea0}
&{APP6_UEAAAAAX9OjpGaUS8hudzB_UQgOsHU_ghvg0Wug fe_related_searches Related queries RELATED_QUERIES_1 0xc0004eb0e0}

But as you can see. when we have multiple ComparisonItem. It created IDs by adding and incrementing an integer to the end.

But in here

gogtrends/vars.go

Lines 23 to 26 in ecf6908

intOverTimeWidgetID = "TIMESERIES"
intOverRegionID = "GEO_MAP"
relatedQueriesID = "RELATED_QUERIES"
relatedTopicsID = "RELATED_TOPICS"

Everything is constant. So when I use compare[4] to get related results. It fails.

relQ, err := gogtrends.Related(ctx, compare[4], "EN")
if err != nil {
	log.Fatal(err)
}

Because it does equality checking in here.

gogtrends/gogtrends.go

Lines 251 to 254 in ecf6908

func Related(ctx context.Context, w *ExploreWidget, hl string) ([]*RankedKeyword, error) {
if w.ID != relatedQueriesID && w.ID != relatedTopicsID {
return nil, ErrInvalidWidgetType
}

Checking the prefix should do good IMO.

@ycd
Copy link
Contributor Author

ycd commented Dec 19, 2020

Linter says it has failed but it also says linter has found no issues, let me know if anything else needed.

golangci-lint found no issues

@groovili
Copy link
Owner

Hi, Yagiz! Thanks for your contribution. Yep, this case was not covered with a package code, so it's a nice update.
Would be good to get this case (multiple ComparisonItem's) covered with unit test, to avoid bugs in the future :)

@groovili
Copy link
Owner

@ycd thanks!

@groovili groovili merged commit 6589704 into groovili:master Jan 11, 2021
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