Skip to content

build: extract helper methods in DownloadBrandLogosTask to reduce complexity#6326

Closed
RubenKelevra wants to merge 1 commit intostreetcomplete:masterfrom
RubenKelevra:refactor-download-brand-logos-task
Closed

build: extract helper methods in DownloadBrandLogosTask to reduce complexity#6326
RubenKelevra wants to merge 1 commit intostreetcomplete:masterfrom
RubenKelevra:refactor-download-brand-logos-task

Conversation

@RubenKelevra
Copy link
Copy Markdown
Contributor

@RubenKelevra RubenKelevra commented Jun 7, 2025

Split run() method into downloadLogo() and computeSmallImageUrl() to improve readability and maintainability.

Fixes two detekt complains:

buildSrc/src/main/java/DownloadBrandLogosTask.kt:20:21: The function run appears to be too complex based on Cyclomatic Complexity (complexity: 19). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]

buildSrc/src/main/java/DownloadBrandLogosTask.kt:20:21: Function run is nested too deeply. [NestedBlockDepth]

…plexity

Split run() method into downloadLogo() and computeSmallImageUrl()
to improve readability and maintainability.
@RubenKelevra RubenKelevra changed the title build: extract helper methods in DownloadBrandLogosTask to reduce com… build: extract helper methods in DownloadBrandLogosTask to reduce complexity Jun 7, 2025
Copy link
Copy Markdown
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Did you test it?

@RubenKelevra
Copy link
Copy Markdown
Contributor Author

Well, I didn't really expect that you're working on the weekend. So the two tasks are still running for validation. Got bamboo piping - how we say in Germany - so this will take a while.

But I noticed that we skip all svgs... that would be easily fixable, as Wikipedia does render SVGs, we would just need to convert the URL from the original SVG file to the rendered PNG ones. Should I add this, or is there no plan to ever use this again?

@matkoniecz
Copy link
Copy Markdown
Member

matkoniecz commented Jun 7, 2025

Well, I didn't really expect that you're working on the weekend.

At least I am appearing on this issue tracker quite randomly, especially as basically all of my issue tracker activity here is an unpaid hobby :)

I would expect that PRs not yet finished and not yet ready for full review and merge would be created as drafts (I did it with say #6302)

@RubenKelevra RubenKelevra marked this pull request as draft June 7, 2025 14:50
@RubenKelevra
Copy link
Copy Markdown
Contributor Author

Sure, let's make it a draft. :)

@RubenKelevra
Copy link
Copy Markdown
Contributor Author

... and I think retries would also help. The current version is failing hard on a single timeout :/


> Task :app:downloadBrandLogos FAILED

[Incubating] Problems report is available at: file:///home/ruben/Projects/streetcomplete_StreetComplete/build/reports/problems/problems-report.html

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:downloadBrandLogos'.
> java.net.ConnectException: Connection timed out

@westnordost
Copy link
Copy Markdown
Member

Maybe the URLs are not current anymore. Or maybe in the meantime there is some sort of (LLM-training-induced) quota on getting these logos.

In any case, this class is basically dead code. We don't actually download the brand logos for StreetComplete, because I noticed that they are just too big to include. The code just sits there because it also doesn't bothers anyone.

So... anyway, let's just not spend time on that.

@westnordost westnordost closed this Jun 7, 2025
@RubenKelevra RubenKelevra deleted the refactor-download-brand-logos-task branch June 7, 2025 16:25
@RubenKelevra
Copy link
Copy Markdown
Contributor Author

Sad, I liked the idea to get small icons, say if you're asked about the opening hours, next to the name of the shop.

I mean, instead of saving all of them inside the app, they could also be fetched on the fly and stored it in the cache of the app instead?

@matkoniecz
Copy link
Copy Markdown
Member

And if it is dead and use is not planned then maybe it can be deleted?

@westnordost
Copy link
Copy Markdown
Member

Well, I guess it would be possible to download them directly when being displayed and then cached for some time. That's how iD does it. It would be unusual though for StreetComplete, because otherwise all functionality is available offline.

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