-
Notifications
You must be signed in to change notification settings - Fork 24
refactor identity usage #111
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
base: feature-reconnect
Are you sure you want to change the base?
refactor identity usage #111
Conversation
startDate: connectionInfo.startDate | ||
}; | ||
|
||
// Don't process dumps generated by JVB & Jigasi, there should be a more formal process to |
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.
bellow this line we persist connections that are JVB and JIGASI these do not go through the FeatureExtractor
instead they are just saved directly, so the dumpData is still needed at this point.
Also line 77 calls this.persistDumpData(dumpData);
I don't see that function defined in this class.
userRegion | ||
}; | ||
|
||
if (typeof applicationName !== 'undefined') { |
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.
with or without the ifs won't this produce the same result? I don't think we need to check each individual value. e.g. we remove the if and the code simply becomes:
this.dumpInfo.conferenceId = confName; this.dumpInfo.app = applicationName; ...
if the values are undefined the values inside dumpInfo will simply be undefined as well which works fine for our purposes. Or do we check somewhere if the actual key exists?
this.collector = new QualityStatsCollector(this.statsFormat); | ||
} | ||
|
||
if (typeof connectionInfoJson?.startDate !== 'undefined') { |
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.
no need to check the typeof simply assign the value to this.dumpInfo.startDate
* Required when creating mock tests. | ||
*/ | ||
constructor({ tempPath, dumpFolder, connectionInfo, log, persistDump = false }) { | ||
constructor({ tempPath, dumpFolder, dumpPath, connectionInfo, log, persistDump = false }) { |
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.
I think tempPath and dumpFolder are no longer needed if we're going to rely on the newly added dumpPath right?
No description provided.