Makes Jenkins/Vulcan a big tad happier about the GUI and its test.
Details
- Reviewers
Nescio Silier - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP23584: Syntax cleanup of GuiInterface and test.
Verify that everything is correct and nothing overlooked.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8328 Build 13598: Vulcan Build Jenkins Build 13597: arc lint + arc unit
Event Timeline
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/55/display/redirect
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
8–10 | -ized | |
65 | which | |
284 | let? | |
420 | Shouldn't it be 2 * Math.PI? | |
528 | idem | |
799 | Aren't comments typically on a new line? (Also below.) | |
1132 | hold | |
1139 | idem |
I did not really change any code (except one var -> let) and, as far as I can tell, you are a very exact person. So yeah, I think you are ;)
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
1132 | What they meant here was probably something like: "If yes, this holds (...)". I'm not even sure these comments (and below) are needed, because it seems obvious from the code, but that's not up to me ;) |
Thanks @Nescio!
- Inlines.
- One var => let.
and always discourage non-core members from doing wholesale commits like this for obvious reasons..
Sorry, didn't know,,,
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/67/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/202/display/redirect
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
1210 | This calculation seems to get the radius so 0.5 is not really a magic value, no? |
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
8–10 | Transltable strings are in en-US, code had been changed from british english to US english too, for example in the colour -> color commits. And Amour had always been planned to be renamed to Armor. | |
955 | (Objects storing positions should be use the Vector prototype, so that one can use these algebraic operations. If it's globally consistent then these operations are available by default and new code would also be introduced with that feature.) | |
1480 | (A bit noisy) |
Havent look into whole file yet.
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
953 | Why brackets for one line ? | |
1006 | I think this one can be nuked | |
1164 | This should be obvious from next lines | |
1402 | This looks not needed same L1427 | |
1621 | this should end with , |
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
136 | i think we do not really need this type of comments | |
336 | please make this kind of object style consistent and put , after last key-value, | |
565 | Doxygen | |
577 | Doxygen | |
603 | Doxygen | |
614 | Doxygen | |
620 | Doxygen | |
1291 | could you change to previewEntites.length && | |
1373 | you moved this, bring it back, you have changed logic | |
1777 | You need to decide if you put those type of comments above else, under else or at the line of next instruction |
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
336 | make it consistent on function level, if count of both occurrences is even, make it consistent on file level :) |
- Rebase.
- Inlines.
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
1210 | Yes, but according to the former comment it was determined through trail and error, so that would imply it is not really purposfully dividing by 2. | |
1358 | [undefined].length == 1 I wonder whether that was intended. (From rP11760.) |
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
422 | 2 * Pi ? (spaces) | |
423 | Could merge the two statements, and ternary for cmpUnit ai like so: ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, cmpUnitAI ? 0 : 2 * Math.PI); One might also wonder why not checking for buildingAI. | |
955 | Is it still worth creating new objects? I still like having vectors though. Wonder if we couldn't just pass pos to cpp | |
1210 | Three patches modifying that line (this one, sockets, restoring turrets) |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1971/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1972/display/redirect
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
68–75 | Better keep the brackets for this type of expression, otherwise diff seems OK. |
after this i can accept
binaries/data/mods/public/simulation/components/GuiInterface.js | ||
---|---|---|
68–75 | yes, better to keep them here for readability and not exactly one line code |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/macos-differential/571/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1991/display/redirect
linter is very happy
changes are mostly cosmetic, they do not affect how code is executed
one branch removal what is nice
changes apply to cc
Thanks for the commit @Angen, we can actually obtain useful information from the linter when modifying any of these two files now :)