[core] Fix offhand equipment resetting TP -- Pending Caps --
I affirm:
- [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
- [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
- [x] I have read and understood the Contributing Guide and the Code of Conduct.
- [x] I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.
What does this pull request do?
- No longer reset tp when attempting to equip an offhand without Dual Wield trait
- No longer unequip a shield if equiped when attempting to equip and offhand without a DW trait
- Add tests to cover the changed cases.
Steps to test these changes
- Run unit tests
- Launch the game, equip a weapon in the main hand as nin 8.
- Gain TP
- Try to equip a valid weapon in the offhand
- observe tp not resetting.
Very solid work providing tests for this change. This sets a great precedent for all PRs going forward.
That being said, there are some very suspect things happening in here. My personal stance is that it's entirely OK to use AI as an assistant while you code, but you should be going over any automated-ish output with a fine-toothed comb, and you should be especially stringent as you're double-checking your work before you submit a PR that's had AI assistance.
Based on some of your previous PRs, and even the comments in this PR itself, there are very different grammatical and logical styles, which is suspect (you've never used ligatures in comments, for instance, and I don't think we use them anywhere in the repo).
I can't say for certain, but multiple people have cast their eyes over this PR and came to the same conclusion: at the very least it's inconsistant with itself.
The logic seems fine, so all I ask for this PR is that it's cleaned up to be more logical in line with the comments I've left.
Heya zach. Thanks for your keen eye here, and yes, as you pointed out I used AI for the tests, which I believe personally is the best value it has when coding.
Pardon me if I did not correct its overly verbose comments. What I reviewed (and corrected through iterating) were things like: do the test cases make sense, are we covering all happy / sad paths etc.. Personally, the extra verbosity does not bother me. I have been skewed a bit by late projects where we tend to do that more than not. I will adjust to this project's style and be more stringent with it.
As far as the actual logic changed (not tests) I wrote and tested that in game (with the different equipment cases) before actually writing the unit tests with AI, so I am pretty confident about it.
The one thing that I did add after the fact, was the shield logic, which I am open to suggestions as personally I am not super happy of the location.
I will remove the verbose comments and then we can talk about the captures as well, which I still need help with.
@xkoredev you do not need to answer every single comment. Just address the issues and ask if you have questions.
Also, rebase to resolve the conflicts
As Zach pointed out, there's a bunch of comments that seem pointless. Try to clean those up too
Done
@xkoredev you do not need to answer every single comment. Just address the issues and ask if you have questions.
Okay sorry I saw this a bit late :). It's how I've operated in the past (at work and in PRs here). Will note for future iterations.
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.