Skip to content

Conversation

gravelweb
Copy link
Contributor

@gravelweb gravelweb commented Jun 9, 2016

This is one step towards fixing Zerg Macro (ggtracker/ggtrackerstack#13, ggtracker/ggtrackerstack#49) and Terran Macro (ggtracker/ggtrackerstack#48)

@dsjoerg
Copy link
Member

dsjoerg commented Jun 9, 2016

Looks good.

Is this ready to roll into production?

What kind of testing has been done?

@dsjoerg
Copy link
Member

dsjoerg commented Jun 9, 2016

The ideal would be

  1. to verify that this commit doesn't affect currently passing tests (in the Testing section of README.md)
  2. to add a test that breaks without this commit

@gravelweb
Copy link
Contributor Author

@dsjoerg I was thinking about that after I powered down last night! I noticed the tests as I was greping through the code, but I totally forgot. What's the magic to run the test suite?

@dsjoerg
Copy link
Member

dsjoerg commented Jun 9, 2016

@gravelweb
Copy link
Contributor Author

@dsjoerg Thanks. I'll try to get it done tonight. Sunday at the latest.

@dsjoerg
Copy link
Member

dsjoerg commented Jun 9, 2016

OK @gravelweb and thanks to @nickelsen we now have CircleCI integration so that any commits will be tested automatically and the test status will be reported here for pull requests.

@gravelweb
Copy link
Contributor Author

Neat! Exciting stuff, guys.

@gravelweb
Copy link
Contributor Author

Created tests, and made implementation safer (per player ability tracker, and clear last ability when finding missing ability).

Still need to get changes into ggpyjobs to not pick up actions that didn't work, but for this project it should be all good.

@dsjoerg
Copy link
Member

dsjoerg commented Jun 11, 2016

@gravelweb should this change go into production, or should it wait until ggpyjobs changes have been made? I don't totally understand what impact this change alone would have on the macro stats users see.

@gravelweb
Copy link
Contributor Author

@dsjoerg This will "fix" Terran and Protoss macro, since that plugin is not explicitly looking for TargetAbilityEvents. It will match on ability_name in the events, and it will find them UpdateTargetAbilityEvent. So for that, if we are not too worried about the extra events, it could go into production

Zerg macro is not affected since we are explicitly looking for 'TargetAbilityEvent', so no change will occur yet here. So here we are indifferent to whether this patch is in production or not.

@dsjoerg dsjoerg merged commit 6fdaf29 into ggtracker:upstream Jun 11, 2016
@dsjoerg
Copy link
Member

dsjoerg commented Jun 11, 2016

OK @gravelweb would you like the honors of updating ggpyjobs to use the new sc2reader you've made? And updating esdb to use the new ggpyjobs? Then I will deploy everything into production and monitor for complaints/errors.

@gravelweb
Copy link
Contributor Author

OK on it!

@gravelweb
Copy link
Contributor Author

Doesn't sc2reader get picked up automagicly on startup? I can't seem to pinpoint where ggpyjobs looks for sc2reader version. It's not where I was expecting (requirements.txt)

@gravelweb
Copy link
Contributor Author

Looking at git changelog, it looks like @nickelsen has got ggpyjobs tracking HEAD of upstream. Not sure how that will work for you when you deploy?

@dsjoerg
Copy link
Member

dsjoerg commented Jun 11, 2016

@gravelweb you are correct. so if i deploy it will automagically pick up the latest sc2reader. huzzah. i will deploy, probably in ~12 hours.

@gravelweb
Copy link
Contributor Author

Do you want to pickup my ggpyjobs changes too, so that ZergMacroTracker can start working too? :)

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.

2 participants