Phrase the retention rationale neutrally - #44
Merged
Conversation
The doc comment said the principal travels for about a month and is the only person who investigates an incident. That bakes a specific person's circumstances into shipped code, which the contract forbids and which stops being true the moment anyone else runs fabric. The reasoning is sound and stays. Only the framing changes: an operator may be away from a machine for a month, so retention must outlast a trip. The number, the floor, the margin and the test are all unchanged. Also neutralised the assertion message in the pinning test for the same reason. No behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs only, no behaviour change. cos flagged this and asked me not to block on it, so it rides on its own.
What was wrong
The doc comment on
DEFAULT_LOG_RETENTION_DAYSsaid "the principal travels for about a month and is the only person who investigates an incident". That bakes a specific person's circumstances into shipped code — which the contract forbids, and which stops being true the moment anyone else runs fabric.The reasoning is sound and stays. Only the framing changes: an operator may be away from a machine for a month, so retention must outlast a trip. The number, the floor, the margin, and the test are unchanged.
Also neutralised the assertion message in the pinning test, which said "before the only person who investigates it gets home".
Something I found while checking, and did not fix here
Grepping for the same class turned up more, all pre-existing and none of it mine:
src/daemon.rs:4680andsrc/service.rs:865— comments attributing a memory-policy decision by personal name ("Nathan's rule: …").src/service.rs— six test fixtures hardcoding a username in sample paths:/home/nathan/.local/share/fabric,/Users/nathan/Fabric 100%.I left them alone. cos asked for a small fix to my own comment, and rewriting six assertions plus two decision-attribution comments in a file I was not asked to touch is scope I should not take unilaterally — the test fixtures carry real regression risk for no functional gain, and the attributions encode who decided something, which may be deliberate.
Flagging rather than fixing or ignoring. Happy to do either on request.
Tests
201 lib tests green.