Page MenuHomeWildfire Games

Corrected hasDealtWithTech in TriggerHelper
AbandonedPublic

Authored by Grapjas on Jun 30 2021, 5:26 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

HasDealtWithTech returned true if the tech was queued up or started, which is offputting if you expect it to only return true if it has been researched like the function name suggest.

Added a Test version of the function, which still does count queued and started researching techs as finished.

Test Plan

Check that hasDealtWithTech only returns true when the tech has finished.
Check that hasDealtWithTechTest returns true when the tech has queued || started || finished.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Grapjas requested review of this revision.Jun 30 2021, 5:26 PM
Grapjas created this revision.
Grapjas added a reviewer: Restricted Owners Package.Jun 30 2021, 5:51 PM
Grapjas updated this revision to Diff 18300.Jul 1 2021, 1:19 PM
Grapjas edited the test plan for this revision. (Show Details)

Added context.

Silier added a subscriber: Silier.Jul 2 2021, 9:15 AM

I dont see where name of the function suggest that technology is researched.
I queued this technology so I dealt with it and now I dont need to care about it.

Grapjas added a comment.EditedJul 2 2021, 11:32 AM
In D4186#178011, @Angen wrote:

I dont see where name of the function suggest that technology is researched.
I queued this technology so I dealt with it and now I dont need to care about it.

It's not obvious to me, in any case it's easily misunderstood. Imo for something to be 'dealt with' means that it's been completed, otherwise it still hasn't been dealt with. Though, I could rename the added function to HasResearchedTech, and revert the original function's name back.

bb added a subscriber: bb.Aug 28 2021, 8:59 PM

A function just returning cmpTechnologyManager.IsTechnologyResearched(techName), if we need it, should be named something like IsTechnologyResearched.

If you have a better name for a function returning cmpTechnologyManager.IsTechnologyQueued(techName) || cmpTechnologyManager.IsTechnologyStarted(techName) || cmpTechnologyManager.IsTechnologyResearched(techName) feel free to propose it.

Grapjas abandoned this revision.Tue, Aug 2, 5:06 PM