Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mirror: Unify Content's EntitySystem logging #240

Conversation

SimpleStation14
Copy link
Member

Mirror of PR #26216: Unify Content's EntitySystem logging from space-wizards space-wizards/space-station-14

eeaea6c25b496106eb741e93738f2ab8503949ba

PR opened by LordCarve at 2024-03-17 20:05:08 UTC


PR changed 13 files with 43 additions and 82 deletions.

The PR had the following labels:

  • Status: Needs Review

Original Body

About the PR

Log things via Log with generated sawmill name rather than an explicitly set one for all Content EntitySystems, except the ones with Rules. In all cases the explicit _sawmill was redundant.

Why / Balance

  • Bringing consistency to logs (all logs originating from EntitySystem should be recognizible in the logs system.something).
  • Logs' sawmills match system class name to assist with debugging.
  • Less likelihood of someone building a new EntitySystem to copy-paste the unnecessary _sawmill and instaed use the appropriate inherited Log instead.

Technical details

This addresses just Content's EntitySystems.

GameRuleSystem and all classes inheriting from it are excluded. I want to include both the calling system name and the rule name in the sawmill name - this however requires engine changes. I thus opted to cut out that part and made this a Content-only PR.

Also, even with the rule changes, the GameRules themselves will be excluded pending antag refactor, because currently they are a hot-hot mess and I'm sure it's preferable by everyone for me to wait for it to cool down before introducing merge conflicts unnecessarily.

Log Sawmill changes:
System class | current master sawmill | this PR's sawmill
VaporSystem.cs: vapor -> system.vapor
DeviceListSystem.cs: devicelist -> system.device_list
ForensicScannerSystem.cs: forensic.scanner -> system.forensic_scanner
MappingSystem.cs: autosave -> system.mapping
MechSystem.cs: mech -> system.mech
LagCompensationSystem.cs: lagcomp -> system.lag_compensation
NpcFactionSystem.cs: faction -> system.npc_faction
EmergencyShuttleSystem.cs: shuttle.emergency -> system.emergency_shuttle
EventManagerSystem.cs: events -> system.event_manager
VendingMachineSystem.cs: vending -> system.vending_machine
SharedAnomalySystem.cs: anomaly -> system.anomaly
SharedDeviceLinkSystem.cs: devicelink -> system.device_link
ThirstSystem.cs: thirst -> system.thirst

Manually tested most (all that I had doubts about) and confirmed that LagCompensationSystem's Log.Level is getting correctly set like this.

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Log sawmill name changes. Any analyzers/other software that processes logs needs to be adjusted to new sawmills. Full list of changes in PR - section Technical details.

@SimpleStation14 SimpleStation14 added the Pull Request Mirror Mirrors a PR from another Repo. Automatically applied by mirror bot label Apr 22, 2024
@SimpleStation14 SimpleStation14 marked this pull request as draft May 4, 2024 21:12
@VMSolidus VMSolidus marked this pull request as ready for review May 12, 2024 03:18
@VMSolidus VMSolidus merged commit cc41b00 into Simple-Station:master May 12, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request Mirror Mirrors a PR from another Repo. Automatically applied by mirror bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants