Skip to content

Conversation

@JSGette
Copy link
Owner

@JSGette JSGette commented Mar 27, 2023

No description provided.

@JSGette JSGette force-pushed the initial_structure branch from fd9258c to fa0d8ea Compare March 27, 2023 18:05
}

fun calculateExecHash(str: String): String {
return MessageDigest.getInstance("SHA-256").digest(str.toByteArray())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should save the the MessageDigest instance instead of getting it each time.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 3 to 7
class MergedSpawnExec(
val listedOutputs: List<String>,
val aEnvVars: Map<String, String>?,
val aInputs: Map<String, Digest>?
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be just a class with: listedOutputs, envVars and inputs? Why do you put in two versions? Just store them in a separate list in main, e.g: spawnSpecsA and mergedSpawnSpecs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if you had to keep this structure (which I don't recommend), you could have this as a data class:

data class MergedSpawnSpec(
    val listedOutputs: List<String>,
    val aEnvVars: Map<String, String>?,
    val aInputs: Map<String, Digest>?,
    var bEnvVars: Map<String, String> = HashMap(),
    var bInputs: Map<String, Digest> = HashMap(),
    var presentInBothExecs: Boolean = false,
)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listedOutputs are the same for both executions but inputs and envVars may be different. So I need to save both to compare them later on or only save those that are different in MergedSpawnExec instance

return Pair(execHash, spawnExec)
}

fun calculateExecHash(str: String): String {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str is used in both the function name as well as in the lambda, so the function variable is shadowed. Better to have different names, because this could lead to confusing code.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +17 to +21
MergedSpawnExec(
spawnExec.second.listedOutputsList,
spawnExec.second.environmentVariablesList.associate { it.name to it.value },
spawnExec.second.inputsList.associate { it.path to it.digest }
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this should be one class, and for pathB you want some other class.

@JSGette JSGette merged commit a6ce62e into main Apr 1, 2023
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.

3 participants