Skip to content

Conversation

tamasdomokos
Copy link

No description provided.

startDate: connectionInfo.startDate
};

// Don't process dumps generated by JVB & Jigasi, there should be a more formal process to

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') {

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') {

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 }) {

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants