Bug Fixed #44
Bug Fixed #44
Conversation
This bug fixes the issue when an expression is deleted but the error plot shows other selected plot if it exists else gives could not find expression statement. Earlier if any selected expression was deleted it just gave that 'could not find expression with id 0', now it only gives when all expressions are deleted.
src/herbie/ExpressionTable.tsx
Outdated
| <button onClick={() => setArchivedExpressions([...archivedExpressions, expression.id])}> | ||
| <button onClick={() =>{ | ||
| setArchivedExpressions([...archivedExpressions, expression.id]); | ||
| setSelectedExprId(-1); |
There was a problem hiding this comment.
Set the correct value here.
|
Great work on this, thank you!! A couple of notes... For the PR title, you should describe the bug/feature, e.g. "Fix ErrorPlot display after expression archive"! You should begin the PR by clearly describing the issue (unless the issue was previously logged). E.g. "Currently, when a selected expression is archived, the error plot displays ...". Also describe the expected behavior: "However, I would expect the first remaining expression to be selected." This helps someone who isn't familiar with the problem understand exactly what is going on. Then describe how this was fixed, e.g. "This PR has the ErrorPlot update the selected expression to avoid the display issue." However, to solve this problem, you should instead set the selectedExpressionId appropriately immediately after archiving, to limit the number of places the selectedExpressionId is being modified and keep reasoning about the value in one place. (See my review comment.) Essentially, please move the logic you added to ErrorPlot into ExpressionTable! Then, in ErrorPlot, assume the value is already correct. If the value is -1, say that no expression is currently selected. (Ideally, we would also change the -1 to undefined everywhere, but it's not worth the effort right now.) |
|
After that, this will be ready to merge |
…om ErrorPlot to ExpressionTable Currently the logic for fixing error plot display after expression archive was done by modifying selectedExpressionId in ErrorPlot and ExpressionTable both. However this can be done in a single modification in ExpressionTable. This comit has updated ExpressionTable by modifying selectedExpressionId over there correctly and removed selectedExpressionId modification from ErrorPlot.
I have updated and pushed as you suggested. Thanks for the help, and I will make sure that now onwards the PR title and description are more informative and easily understandable. |
This bug fixes the issue when a selected expression is deleted and other expression exists still the error plot gives, 'Could not find expression with id 0'. So now whenever a selected expression is deleted we change selected id to the next expression in the list and by that the plot doesn't goes and stays until all expression are not deleted.