Improvements to the start up logging for the server list filter #2647
Conversation
This file contains 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
hoffie
approved these changes
Jun 14, 2022
| } | ||
|
|
||
| // assume directoryType will get set to AT_CUSTOM | ||
| if ( !strDirectoryAddress.compare ( "localhost", Qt::CaseInsensitive ) || !strDirectoryAddress.compare ( "127.0.0.1" ) ) |
There was a problem hiding this comment.
Just a gut feeling and nothing blocking for this PR: It feels like those "are we a directory" checks are repeated in many places and might benefit from being moved to a helper function.
There was a problem hiding this comment.
There's a method for it -- but it does an extra check on the directoryType. Because we're not really a directory unless directoryType is set to AT_CUSTOM. Here, however, we explicitly do not care about directoryType - but do care about the address.
ann0see
reviewed
Jun 15, 2022
pljones
force-pushed
the
patch/server-listfilter-logging
branch
from
2580942 to
6cb7174
Compare
That gets picked up automatically, doesn't it, if there's no replacement given? |
pljones
force-pushed
the
patch/server-listfilter-logging
branch
from
6cb7174 to
06b49f8
Compare
ann0see
approved these changes
Jun 18, 2022
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.
Short description of changes
Improvements to the start up logging for the server list filter.
Context: Fixes an issue?
No.
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Could do with testing from people who use the server list part of the feature - I've been running with the version filter logging for months without issue.
What is missing until this pull request can be merged?
Review. I'm not sure who now uses the server list filter (I don't think @sthenos does any more). EDIT: Apparently they do.
Checklist