Author Topic: [DONE][Suggestion] Prison overview, similar to existing research and production  (Read 4361 times)

Offline Yankes

  • Commander
  • *****
  • Posts: 3240
    • View Profile
Roger that. I'll make a new branch with one commit in it. 
Technically you could can have multiple commits BUT each one need be self contained change, not "Add X" and then 3 times "Fix X" and "Fix again X".
https://www.aleksandrhovhannisyan.com/blog/atomic-git-commits/

Correct PR could have commits like:
"White space cleanups" (100 files changed 0 code touch)
"Refactor Y" (10 files changed, lot of code changed but game logic do not change)
"Add X" (1 file, your feature that change game logic)

Offline DoxaLogos (JG)

  • Colonel
  • ****
  • Posts: 358
  • Squaddie cautiously peering through the breach
    • View Profile
Technically you could can have multiple commits BUT each one need be self contained change, not "Add X" and then 3 times "Fix X" and "Fix again X".
https://www.aleksandrhovhannisyan.com/blog/atomic-git-commits/

Correct PR could have commits like:
"White space cleanups" (100 files changed 0 code touch)
"Refactor Y" (10 files changed, lot of code changed but game logic do not change)
"Add X" (1 file, your feature that change game logic)

Thanks again for the tips :)

Just makes it confusing when one repository admin says 1 PR = 1 Commit.  Doesn't leave much room to work with.

I'll see what I can do.
« Last Edit: March 08, 2023, 06:56:18 pm by DoxaLogos (JG) »

Offline Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 8709
    • View Profile
You didn't do any whitespace cleanups or refactoring.
So it's still one commit.

I don't want to sound like my grandma, but just use common sense.
If you had to maintain this whole madness, would you appreciate if people gave you 10 broken pieces instead of one nice package?
(Have you used 'git blame' before? If yes, you will know how much more difficult it is to search for anything in the git history, if you don't follow these guidelines)

At the end, I would just squash your commits into one anyway, so the repo would be safe... but you wanted to help me, not give me more work.

Offline DoxaLogos (JG)

  • Colonel
  • ****
  • Posts: 358
  • Squaddie cautiously peering through the breach
    • View Profile

At the end, I would just squash your commits into one anyway, so the repo would be safe... but you wanted to help me, not give me more work.

Will do then...

Offline DoxaLogos (JG)

  • Colonel
  • ****
  • Posts: 358
  • Squaddie cautiously peering through the breach
    • View Profile
Here's what it looks like with Yanke's group suggestion.

If everyone thinks it looks good enough for merging, let me know. I'll start work on squashing my commits.
« Last Edit: March 08, 2023, 09:16:05 pm by DoxaLogos (JG) »

Offline DoxaLogos (JG)

  • Colonel
  • ****
  • Posts: 358
  • Squaddie cautiously peering through the breach
    • View Profile
Alright... second go around.  Here's a PR for this feature request: https://github.com/MeridianOXC/OpenXcom/pull/116

Offline Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 8709
    • View Profile
Substandard quality unfortunately.

1. shows that I have aliens in the Access Lift
2. translations are maintained in Transifex, not in the source code (otherwise they'll just be lost with the next update)
3. missing files for Visual Studio project
4. empty methods (e.g. onOpenTechTreeViewer)
5. unnecessary includes (e.g. ResearchProject)
6. unnecessary data copying (e.g. for(auto itemName)
7. if I understand correctly, it doesn't cover the case when multiple facilities use the same prison type

I'll need to review line by line before merge.

Offline DoxaLogos (JG)

  • Colonel
  • ****
  • Posts: 358
  • Squaddie cautiously peering through the breach
    • View Profile
I can go back and rework it.  This is why we have a "review" process.

1.  I didn't realize that the access lift had a prison type attached.  EDIT:  After further thought, I just now realize that every facility without a prison type gets a prison type of 0 added to it for backwards compatibility.  In this case, this type 0 is 'aliens'.  I'm not sure how to work around this.  Maybe the capacity(i.e. "aliens")?  Which might still be a problem as they get a capacity of zero.

2.  This is an area I definitely don't know how to use, so I could probably use some pointers.
3. Yep, sorry about that. I build on Linux.  I don't have a windows setup.

4 & 5.  I know why that happened.  I can fix.

6.  I'll have to look at that again.
7.  It doesn't, and I think I know where I missed this.

EDIT: Thanks for the time to provide feedback.  I do appreciate it.
« Last Edit: March 16, 2023, 04:36:49 pm by DoxaLogos (JG) »

Offline Meridian

  • Global Moderator
  • Commander
  • *****
  • Posts: 8709
    • View Profile
Merged.

Here's the default translations for modders:

Code: [Select]
  STR_PRISONER_OVERVIEW: "PRISONER OVERVIEW"
  STR_PRISONER: "PRISONER"
  STR_PRISONER_AMOUNT: "AMOUNT"
  STR_PRISONER_INTERROGATED: "INTERROGATED"
  STR_TOTAL_IN_PRISON: "Space Used>{ALT}{0}"
  STR_TOTAL_INTERROGATED: "Interrogated>{ALT}{0}"
  STR_PRISON_TYPE: "Alien Containment"

Additional prison types can be translated like this:

Code: [Select]
  STR_PRISON_TYPE_1: "Aquarium"
  STR_PRISON_TYPE_2: "Prison"
  STR_PRISON_TYPE_3: "Junkyard"

Offline DoxaLogos (JG)

  • Colonel
  • ****
  • Posts: 358
  • Squaddie cautiously peering through the breach
    • View Profile
Oh wow.  Thanks.  I was going go back and rework things so you didn't have too.