-
Notifications
You must be signed in to change notification settings - Fork 1
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
EAGLE-1431: Stop EAGLE overwriting modelData when loading palettes #850
base: master
Are you sure you want to change the base?
Conversation
…by eagleServer, and by EAGLE itself
…ed some static text out of data-bindings.
Reviewer's Guide by SourceryThis pull request addresses issues related to palette loading and model data display. It prevents EAGLE from overwriting modelData and re-sorting nodes when loading palettes. Additionally, it updates the modelData modal to include generator information and fixes a bug where the download URL was not displayed. Sequence diagram for loading palettessequenceDiagram
participant Eagle
participant Palette
Eagle->>Palette: loadPalette(paletteData)
activate Palette
Palette->>Palette: fileInfo().modified = true
deactivate Palette
Updated class diagram for FileInfoclassDiagram
class FileInfo {
-_generatorName: string
-_generatorVersion: string
-_generatorCommitHash: string
-_downloadUrl: string
}
note for FileInfo "Renamed eagleVersion to generatorVersion, added generatorName, fixed bug where downloadUrl was not displayed"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @james-strauss-uwa - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to
eagleServer.py
look like they could be simplified by usingdict.update()
to overwrite the model data. - Consider adding a unit test for the bug fix related to
modelData.downloadUrl
.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
nice, I assume that means were treating the sort function as a conscious decision of wanting to to alter the real order of the nodes in the palette?
Also stop EAGLE re-sorting the nodes in a palette after adding nodes from a graph.
Re-named fields in modelData modal from eagleVersion to generatorVersion, add generatorName.
Fixed bug where modelData.downloadUrl was not displayed.
Summary by Sourcery
Improve model data handling and display in EAGLE by updating metadata fields, preventing unintended overwrites, and fixing display issues
Bug Fixes:
Enhancements:
Chores: