feat: remove operations array from the DRP #292
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
JanLewDev
requested review from
trungnotchung,
d-roak and
anhnd350309
as code owners
Comment on lines
+135
to
+143
| const drp = cloneDeep(this.originalDRP); | ||
| const state = cloneDeep(preOperationState); | ||
|
|
||
| for (const [key, value] of state.entries()) { | ||
| drp[key] = value; | ||
| } | ||
|
|
||
| const normalizedArgs = Array.isArray(args) ? args : [args]; | ||
| drp[fn](...normalizedArgs); |
There was a problem hiding this comment.
you can seperate _computeState to _computeDRP(dependencies, op) and getDRPState(drp) to avoid duplicate these code.
Comment on lines
+145
to
+153
| let stateChanged = false; | ||
| for (const [key, value] of preOperationState.entries()) { | ||
| try { | ||
| deepStrictEqual(value, drp[key]); | ||
| } catch (e) { | ||
| stateChanged = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
create a new function to keep this callFn not too long
Comment on lines
+161
to
+166
| const varNames: string[] = Object.keys(drp); | ||
| // biome-ignore lint: values can be anything | ||
| const newState: Map<string, any> = new Map(); | ||
| for (const varName of varNames) { | ||
| newState.set(varName, drp[varName]); | ||
| } |
There was a problem hiding this comment.
separate _computeState can avoid this
Comment on lines
+64
to
+65
| ["grant", "revoke"].includes(vertices[0].operation.type) && | ||
| ["grant", "revoke"].includes(vertices[1].operation.type) |
There was a problem hiding this comment.
it's like the operations array, let's try other solutions
anhnd350309
reviewed
Jan 1, 2025
Comment on lines
+112
to
+117
| if ( | ||
| !(propKey as string).startsWith("_") && | ||
| !(propKey as string).startsWith("is") && | ||
| !(propKey as string).startsWith("get") && | ||
| !(propKey as string).startsWith("query") | ||
| ) |
There was a problem hiding this comment.
Suggested change
| if ( | |
| !(propKey as string).startsWith("_") && | |
| !(propKey as string).startsWith("is") && | |
| !(propKey as string).startsWith("get") && | |
| !(propKey as string).startsWith("query") | |
| ) | |
| const excludedPrefixes = ["_", "is", "get", "query"]; // make this a constant, change this when adding a new prefix | |
| if (!excludedPrefixes.some(prefix => (propKey as string).startsWith(prefix))) |
refactor code
anhnd350309
reviewed
Jan 1, 2025
Comment on lines
+278
to
+286
| if (state) { | ||
| this.states.set(vertex.hash, { state }); | ||
| } else { | ||
| const newState = this._computeState( | ||
| vertex.dependencies, | ||
| vertex.operation, | ||
| ); | ||
| this.states.set(vertex.hash, { state: newState }); | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (state) { | |
| this.states.set(vertex.hash, { state }); | |
| } else { | |
| const newState = this._computeState( | |
| vertex.dependencies, | |
| vertex.operation, | |
| ); | |
| this.states.set(vertex.hash, { state: newState }); | |
| } | |
| if (state) { | |
| this.states.set(vertex.hash, { state }); | |
| return; | |
| } | |
| const newState = this._computeState( | |
| vertex.dependencies, | |
| vertex.operation, | |
| ); | |
| this.states.set(vertex.hash, { state: newState }); |
Reduced Nesting, Readability
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.
Description
This PR closes #257 and closes #310, removing the need to define the operations array by the DRP devs.
Key features:
add(1)is not creating a new vertex_*,get*,is*orquery*. By convention such functions are not creating operations