Merged
Conversation
Collaborator
Author
|
Will force push without the code formatting, as there are too many changes to review |
JunAishima
approved these changes
Nov 13, 2023
Collaborator
JunAishima
left a comment
There was a problem hiding this comment.
Overall, looks good. There are also a few other positive aspects:
- A lot of args/kwargs preserve numerical type, if the value isn't coming from a GUI text box
- Get rid of a fair bit of ugly old-style string assembly, before f strings
- generate_server_message() is well-composed to handle command, args, kwargs to allow full flexibility of calls as with the previous method
- no ispybLib commands are on the whitelist, except for the insertRasterResult() so it shouldn't be possible to do the full cycle of adding yourself to a different proposal. you probably could add your collection to another experiment, but that won't be too useful (but would need to reverse-engineer all of the information in the collection request to do so)
- Do you think we could just extract the send_to_server() function so that one function will be called to generate the message, log, then send to the appropriate queue?
JunAishima
reviewed
Nov 14, 2023
Collaborator
JunAishima
left a comment
There was a problem hiding this comment.
Just want some more explanation for why the generate_server_message() needed to be brought into the ControlMain class
danielballan
approved these changes
Nov 14, 2023
…into jsonize-server-comm
danielballan
approved these changes
Nov 14, 2023
This file contains hidden or 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
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.
Added a mechanism to replace exec and function calls with a structured json that can only execute functions in a whitelist