-
Notifications
You must be signed in to change notification settings - Fork 57
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
Digital Twins page preview - trigger pipeline from client #891
Digital Twins page preview - trigger pipeline from client #891
Conversation
…ture/distributed-demo
…ture/distributed-demo
…ture/distributed-demo
…ture/distributed-demo
… feature/distributed-demo
.replace(/-/g, ' ') | ||
.replace(/^./, (char) => char.toUpperCase()); | ||
|
||
const ExecuteDigitalTwin: React.FC<{ name: string }> = (props) => { |
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.
Function ExecuteDigitalTwin
has 177 lines of code (exceeds 25 allowed). Consider refactoring.
.replace(/-/g, ' ') | ||
.replace(/^./, (char) => char.toUpperCase()); | ||
|
||
const ExecuteDigitalTwin: React.FC<{ name: string }> = (props) => { |
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.
Function ExecuteDigitalTwin
has a Cognitive Complexity of 46 (exceeds 5 allowed). Consider refactoring.
import tabs from './DigitalTwinTabData'; | ||
import ExecuteTab from '../../components/tab/ExecuteTab'; | ||
|
||
function DTContent() { |
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.
Function DTContent
has 38 lines of code (exceeds 25 allowed). Consider refactoring.
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.
Partial code review done. Please remember to run yarn format
and yarn syntax
before committing your changes.
@VanessaScherma Thanks for the PR. Please check the suggestions. |
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.
@VanessaScherma This round of comments completes the code review.
|
||
const ExecuteDigitalTwin: React.FC<{ name: string }> = (props) => { | ||
const [gitlabInstance, setGitlabInstance] = useState<GitlabInstance | null>(null); | ||
const [digitalTwin, setDigitalTwin] = useState<DigitalTwin | null>(null); |
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.
Please simplify these react component properties using react redux store. There are some good examples.
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.
Should redux store only be used with gitlabInstance and digitalTwin properties?
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.
Do not understand the question. Can you please elaborate?
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.
In this case, the use of redux store does not seem to be usable, as the properties of this component are not global, but concern only one DT. Using redux store would result in continuous overwriting.
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.
Is there a clean solution to this long list of component state?
@VanessaScherma |
@prasadtalasila |
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.
please add only the asset components needed for your PR. For example, AddButton component is not required for now.
} | ||
|
||
function AssetCardExecute({ asset }: AssetCardExecuteProps) { | ||
const [digitalTwin, setDigitalTwin] = useState<DigitalTwin>( |
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.
This probably does not belong to component state. Please rethink about this design choice.
@prasadtalasila I used the command |
|
So the process would be: within the distributed-demo branch, do |
} | ||
}; | ||
|
||
export const checkFirstPipelineStatus = async ( |
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.
Similar blocks of code found in 4 locations. Consider refactoring.
} | ||
}; | ||
|
||
const handlePipelineCompletion = async ( |
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.
Similar blocks of code found in 4 locations. Consider refactoring.
); | ||
}; | ||
|
||
const retryPipelineCheck = ( |
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.
Similar blocks of code found in 4 locations. Consider refactoring.
); | ||
}; | ||
|
||
export const checkSecondPipelineStatus = async ( |
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.
Similar blocks of code found in 4 locations. Consider refactoring.
Code Climate has analyzed commit 94614ec and detected 8 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
I am working on the required tests and the only tests left are for the file ExecutionFunctions.ts. However, I am experiencing a problem that I cannot solve. `Jest encountered an unexpected token
I even tried running an isolated test to verify that the library is loaded correctly, but the test fails and always returns the library index file, pointing out how the import is wrong. Thank you. |
PR #903 includes the changes suggested here. |
Resolves issue #794. It includes a new link in ‘Workbench’ that gives access to the preview of the Digital Twins page. At the moment, only the ‘Execute’ tab has been developed.