Closed senshi-x closed 11 months ago
NeutraleNull
commented
11 months ago looking pretty good already. Could you try to revert the indent size? It changes the files all over the place. What branch are you working from? I see you have modified files that are not related to this PR?
senshi-x
commented
11 months ago Hence the "draft" status. Editorconfig will of course be changed back when ready for review, I just hate working with the 4 spaces in indent/block-heavy languages such as sqf (so much wasted real estate).
It's based off main branch, which is why the PR is targeting that as well. The only "other" file I changed right now is the lg postinit, because the eventhandler was interfering with my testing. Again, a thing that will change when I submit for review.
Same with description.ext changes, those are for debugging and won't be in here when it goes for review.
b-mayr-1984
commented
11 months ago Thx @senshi-x I will have a look at it in a few days. 🙂
senshi-x
commented
11 months ago I recommend waiting until I mark it as ready for review, so you don't have to wade through all the debug junk 😅 . Shouldn't take me long.
I feel like you guys have a different understanding of "draft" status in github than I do. 🙈 I use it to publish a description of "what I am working on", to prevent someone else from wasting energy on the same topic, and if that someone wants to dive into the same topic, they can look at WIP code if they really want to.
b-mayr-1984
commented
11 months ago I recommend waiting until I mark it as ready for review, so you don't have to wade through all the debug junk 😅 . Shouldn't take me long.
I feel like you guys have a different understanding of "draft" status in github than I do. 🙈 I use it to publish a description of "what I am working on", to prevent someone else from wasting energy on the same topic, and if that someone wants to dive into the same topic, they can look at WIP code if they really want to.
Ah okay. Good to know. When I see a pull request I assume it is meant to be checked already. I will wait then.
senshi-x
commented
11 months ago Ready for review. @NeutraleNull Had to fix EH handling in lgshells that was broken (idx of GetInMan instead of FiredEH was used to later attempt to remove FiredEH). Which obviously messes it up for everyone.
Sorry for some "useless" line changes being shown, mostly whitespace cleanups. Apparently your guys' editor doesn't enforce editorconfig's trim_trailing_whitespace=true?
senshi-x
commented
11 months ago Most of the "empty" changes result from using an IDE that honors editorconfig (by automatically enforcing/applying it), as also mentioned in https://github.com/gruppe-adler/Shoot_and_Scoot.Tanoa/pull/27#issuecomment-1637795166 Sure, those changes are only cosmetic and not functional changes, but I feel you can't really blame me for cleaning up code by accident. And I really don't feel like having to make code "dirtier" purely to reduce the amount of changed lines.
If the 2 space indent bothers you, I can change that, sure. I do not know what you mean by "density and style of code comments", so not sure what you need changed there.
b-mayr-1984
commented
11 months ago Most of the "empty" changes result from using an IDE that honors editorconfig (by automatically enforcing/applying it), as also mentioned in #27 (comment)
I am aware of that. I read your comment there.
Sure, those changes are only cosmetic and not functional changes, but I feel you can't really blame me for cleaning up code by accident.
I don't want to blame you for anything. I just explain the reasons for me having trouble to review and get an understanding of your code.
And I really don't feel like having to make code "dirtier" purely to reduce the amount of changed lines.
You don't have to make due with what you perceive as "dirty code". But putting behavioral changes in the same PR as refactoring changes makes it (unnecessarily) hard for a maintainer. Particularly if so many files and so many LOCs have been changed all at once.
If the 2 space indent bothers you, I can change that, sure.
That would really help, yes.
I do not know what you mean by "density and style of code comments", so not sure what you need changed there.
I was a bit too general with my statement here. When I was writing this I was looking at fn_onFired.sqf.
This file is very compressed. It does not make much use of elements to structure the code visually (e.g. aiding the eye by means of empty lines where functional blocks end or placing [ and { etc. so that one can see where a closing bracket is that belongs to an opening one).
If you look at other code files in this repository they look very different to this one and they make way more use of code comments.
To just add a little context:
In my estimation this is what reviews are for. To see how others perceive changes. If others struggle there are probably reasons for it, and those reasons I am trying to present from my point of view.
b-mayr-1984
commented
11 months ago Most of the "empty" changes result from using an IDE that honors editorconfig (by automatically enforcing/applying it), as also mentioned in #27 (comment)
I am aware of that. I read your comment there.
Since this also caused confusion in the past (see Diwako's PR) and since my editor of choice (Notepad++ 👴) does not support it out of the box (but only with a plugin) I am trying to work with Visual Studio Code in the future.
Maybe this makes things easier for future PRs.
senshi-x
commented
11 months ago How you want to code is entirely up to you, nobody likes being restricted in their methodology. But if you want people to adhere to a "codestyle", it makes sense to define a codestyle first. And editorconfig is a great way to do it in a way so people can still work with whatever IDE they prefer. Which is why I'm absolutely fine with moving to 4 space indents, even if I dislike them. If your IDE does not enforce it, it still is not mandatory for you to change anything or QA your own code, but then you'll definitely have to keep dealing with "driveby" cleanups in other PRs such as mine. ;)
b-mayr-1984
commented
11 months ago How you want to code is entirely up to you, nobody likes being restricted in their methodology.
Yes and no. I am used to work in larger development teams. This brings with it that individual freedom of methodology gets restricted by the need to effectivly work together. So I am fine with accepting some restrictions.
But if you want people to adhere to a "codestyle", it makes sense to define a codestyle first.
I don't really care about a specific codestyle. I care about being able to maintain the source code. This becomes easier if one project is not littered with different codestyles.
And editorconfig is a great way to do it in a way so people can still work with whatever IDE they prefer.
I agree. My own professional coding days lie more than 10 years in the past though. I've built my coding muscle memory with editors that become increasingly obsolete with every day that passes. So in the spirit of teamwork I should move on rather than clinging to the old days. Even more so since with more people contributing here, my toolchain might become an increasing problem (e.g. by not supporting EditorConfig out of the box).
Which is why I'm absolutely fine with moving to 4 space indents, even if I dislike them. If your IDE does not enforce it, it still is not mandatory for you to change anything or QA your own code,
👍
but then you'll definitely have to keep dealing with "driveby" cleanups in other PRs such as mine. ;)
Sorry, no! 🙅 Such a "driveby" action makes the review process to difficult. Delta-evaluation just becomes a mess. It is way better to separate functional changes and refactoring changes into separate change items (e.g. separate PRs). Just so that the reviewers/maintainers don't have to do so much reverse engineering of what you actually (functionally) changed.
Is there no way for you to revert the cosmetical changes back to how it was in the beginning? You should have a way easier time doing that compared to me that hasn't actually implemented the changes. I could easily overlook changes that are functional, but appear cosmetical to me, since I am not immersed in your thought processes.
senshi-x
commented
11 months ago I have changed the indents to 4 spaces to match editorconfig.
A suggestion to make reviewing this PR easier for you: In the "Files changed"-view of github, you can disable whitespace diffs. (small cog icon top left). Then you will only see "functional" changes. A similar function (ignore whitespace) should exist for most IDEs or git diff tools out there if you don't use github's webview.
A second suggestion: If preferred, we can just review this PR together sometime in TS/Discord. Might be easier especially if you have many questions and input regarding my code.
I will not undo any whitespace cleanup in this PR. That's a backwards approach.
I agree with you on the principle that refactor and functional changes should be separated. In this case, I didn't really refactor anything (renames), it's just whitespaces, so I feel it can be pragmatically included in this PR. If you disagree, the "proper" way to deal with this would be to simply do the whitespace cleanup in a separate PR and merge that first. Then the diff between main and this PR will no longer include those whitespace changes.
I also added another feature: tasks & markers get updated (for zeus/streamer only) once objectives are destroyed so they can easily track objective progress.
Code is ready for re-review.
b-mayr-1984
commented
11 months ago I have changed the indents to 4 spaces to match editorconfig.
👍
A suggestion to make reviewing this PR easier for you: In the "Files changed"-view of github, you can disable whitespace diffs. (small cog icon top left). Then you will only see "functional" changes. A similar function (ignore whitespace) should exist for most IDEs or git diff tools out there if you don't use github's webview.
Yes, using Visual Studio Code along with GitLens extension, as well as the setting that you mentioned, already makes my life way easier. I just need to get used to the new tools though.
A second suggestion: If preferred, we can just review this PR together sometime in TS/Discord. Might be easier especially if you have many questions and input regarding my code.
That would be great, yes. 🙂
I will not undo any whitespace cleanup in this PR. That's a backwards approach.
Oh well, (reluctantly) accepted. 😐
I agree with you on the principle that refactor and functional changes should be separated. In this case, I didn't really refactor anything (renames), it's just whitespaces, so I feel it can be pragmatically included in this PR. If you disagree, the "proper" way to deal with this would be to simply do the whitespace cleanup in a separate PR and merge that first. Then the diff between main and this PR will no longer include those whitespace changes.
Exactly! 👍
I also added another feature: tasks & markers get updated (for zeus/streamer only) once objectives are destroyed so they can easily track objective progress.
I appreciate the feature. It implements something that was on my todo list and get's us closer to an automated mission end. 🙂
From the review process point of view I have to admit though that it doesn't get easier with more/bigger changes. Having things separated and digestable in smaller bites would make it easier for maintainers.
Code is ready for re-review.
I'll have a look at it. Thx 🙂
These are some QoL changes and are not supposed to impact gameplay.
Motivation/Goals:
Shelltracker
This module allows qualified players (currently only curators) to visualize artillery shells on the map.
Features
Container Notify Damage
Adds a handleDamage EH that triggers a simple systemchat for Zeus players to inform them about containers taking damage. Might later also be used to automatically track target status, e.g. for mission end trigger. Example:
Blue 6 was hit by Senshi for 53.6883 % with Sh_82mm_AMOS . Current HP: 28.0927 %