-
-
Notifications
You must be signed in to change notification settings - Fork 85
feat(skymp5-server): create SweetHidePlayerNamesService #2578
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
Open
Pospelove
wants to merge
6
commits into
skyrim-multiplayer:main
Choose a base branch
from
Pospelove:stranger
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to e9c756a in 1 minute and 59 seconds. Click for details.
- Reviewed
14lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skymp5-server/cpp/server_guest_lib/PartOne.cpp:777
- Draft comment:
Using pointer equality (emitter != listener) to check for self may be error-prone. Consider comparing unique actor IDs instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is making a suggestion about code quality/robustness, claiming pointer comparison is "error-prone." However, this is speculative - there's no evidence that pointer equality is actually problematic here. The code appears to be checking if the emitter and listener are literally the same object instance, which is a valid use of pointer equality. The comment uses "may be error-prone" which is speculative language. It also suggests an alternative (comparing unique actor IDs) but doesn't explain why the current approach is wrong or what specific error could occur. This seems like a "could be better" suggestion rather than identifying a definite bug. The pattern is used consistently throughout the function, suggesting it's intentional. Perhaps there's a legitimate concern about object lifetime or pointer stability that I'm not seeing from this limited context. Maybe in this codebase, objects can be recreated with different pointers but same IDs, making pointer equality unreliable. Even if there were such concerns, the comment doesn't provide evidence of this being an actual issue. It uses speculative language ("may be error-prone") and doesn't demonstrate a concrete problem. Without strong evidence that this is definitely wrong, and given that the same pattern is used multiple times in the same function, this appears to be speculative advice rather than identifying a clear bug. This comment should be deleted. It's speculative ("may be error-prone"), doesn't provide evidence of an actual problem, and suggests a refactor without demonstrating why the current approach is incorrect. The pointer equality pattern is used consistently throughout the function, suggesting it's intentional and working as designed.
2. skymp5-server/cpp/server_guest_lib/PartOne.cpp:777
- Draft comment:
Avoid hardcoding the 'Stranger' string; consider using a configurable constant for maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a code quality suggestion about extracting a magic string to a constant. The change is definitely in the diff (lines 777-779). The suggestion is actionable and clear - the author could extract "Stranger" to a named constant. However, I need to consider: 1) Is this obvious or unimportant? It's a single string used once, so some might argue it's not critical. 2) Is this clearly a code change that's required? The rules say "Do NOT comment unless there is clearly a code change required" and to only keep comments with "STRONG EVIDENCE" that they're correct. This is more of a "nice to have" refactoring suggestion rather than a bug or clear issue. 3) The string appears only once in this location, so the maintainability argument is weaker than if it were used multiple times. This is a minor code quality suggestion about a single-use string literal. While extracting magic strings to constants is generally good practice, this particular case involves a string used only once in one location. The comment doesn't identify a bug or critical issue, just a potential improvement. The rules emphasize keeping only comments where there's "STRONG EVIDENCE" of correctness and clear code changes required. While the suggestion is technically valid and follows good practices, it's borderline whether this rises to the level of "clearly a code change required." The string is used once, the intent is clear from the inline comment, and there's no indication this needs to be configurable or reused elsewhere. This falls more into the "nice to have" category rather than a clear issue. This comment should be deleted. While it's a reasonable code quality suggestion, it's too minor for a single-use string literal and doesn't meet the threshold of "clearly a code change required." The inline comment already explains the purpose, and there's no strong evidence that making this configurable is necessary.
Workflow ID: wflow_N5tTLjq9VBKIEX0t
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Important
In
PartOne::Init(), hide other players' names by settingmessage.appearance->nameto "Stranger" whenemitteris notlistener.PartOne::Init(), setmessage.appearance->nameto "Stranger" ifemitteris notlistenerto hide other players' names.This description was created by
for e9c756a. You can customize this summary. It will automatically update as commits are pushed.