AdiAddons / LibPlayerSpells-1.0

WoW addon library - additional information about player spells.
GNU General Public License v3.0
14 stars 23 forks source link

Battle for Azeroth Warrior #127

Closed VaporAPX closed 6 years ago

VaporAPX commented 6 years ago

TO REVIEW in the coming beta patches: Protection:

EDIT: In the latest build, revenge now reduces rage on Ignore Pain, but the buff is not consumed on using Ignore Pain. Ignore Pain does not reduce the rage of Revenge.

Still no snare from charge, and intercept stops short of melee.

VaporAPX commented 6 years ago

Wtf, I saved this one with removing trailing white spaces and it still failed the check.... I didn't have this issue until recently, don't know what changed.

Rainrider commented 6 years ago

It is failing because of mixed indentation (tab after an empty space) at line 56.

TL;DR Don't worry, I can fix that later with the press of a button :)

I was wondering why it wasn't failing before, it turned out the tests ran in the wrong folder. I fixed it in the bfa branch and that's the branch the tests are run against.

The tests are quite rudimentary though, For indentation it looks only for spaces before tabs, so you can indent wrongly (i.e. spaces only or wrong indent level) and still pass.

Basically the idea is to somewhat enforce consistent code style and reduce history pollution. To git even a single white space is a change and such changes are most of the time irrelevant and just waste time when reviewing commits.

Some linters like luacheck perform trailing white space and indentation checks. You can also run the test locally before committing. However this will require further setup which is not that straightforward on Windows.

VaporAPX commented 6 years ago

@Rainrider All warrior abilities and talents are in other than honor talents. Hopefully a little cleaner than paladin lol.

VaporAPX commented 6 years ago

Just found a bug in testing. The protection talent Vengeance doesn't currently work on beta. Ignore pain isn't buffing with "Vengeance: Revenge" and does not reduce the rage cost of revenge.

Rainrider commented 6 years ago

Reviewed protection. Very well done! I just added POWER_REGEN flag (can't add Boomind Voice here sadly as LPS does not have conditional flags) and the RAIDBUFF flag, which was added to LPS after you started work on warrior.

Just found a bug in testing. The protection talent Vengeance doesn't currently work on beta. Ignore pain isn't buffing with "Vengeance: Revenge" and does not reduce the rage cost of revenge.

It appears to be a typo: "Shield Block" applies "Vengeance: Revenge" when "Vengeance" is known. You catched this already.

Though "Intercept" on enemies does not apply the movement speed reduction as stated in the tooltip.

Once again, very good on Protection, thank you for the help!

VaporAPX commented 6 years ago

I've submitted both of those bugs to blizzard already, the only 2 I've found so far. Ignore pain was just added back in and in the previous patch shield block was working with vengeance, so they must have added IP back in and forgot to adjust the talent.

VaporAPX commented 6 years ago

Was gonna do some updating tonight, but OuF broke in this current build and I'm not sure how to fix it lol. The unit frames I use make it super easy to see debuffs and such, so I'll probably wait until tomorrow to tinker.

Rainrider commented 6 years ago

I'll have time to fix oUF tonight, maybe p3lim or ls will be faster than me. It is a trivial problem there.

Rainrider commented 6 years ago

Incomming fury changes: https://us.battle.net/forums/en/wow/topic/20764336416?page=3#53 Will have to wait for a future build for those.