Skip to content
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

Don't return on error #51

Merged
merged 25 commits into from
Aug 23, 2024
Merged

Don't return on error #51

merged 25 commits into from
Aug 23, 2024

Conversation

foodprocessor
Copy link
Collaborator

@foodprocessor foodprocessor commented Aug 22, 2024

Describe your changes in brief

Returning an error from settingsReceived() stops the UI from functioning normally, so settingsReceived() should only return an error when it can't return a valid SettingsResponse.

This PR also includes some refactoring:

  • broke settingsReceived() into multiple functions to improve readability
  • on Windows, verify unmount is complete before moving on
  • check validity of JSON before returning settingsResponse
  • comment out call to processActiveSettings, which does nothing but cause the occasional error. We'll use it properly soon.
  • removed our code from the Engine superclass (in helpers), into our inheriting class.
  • Use m_ prefix for class member fields.
  • Improved Windows build script (faster iteration time)

What type of Pull Request is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Checklist

  • Tested locally
  • Added new dependencies
  • Updated documentation
  • Added tests

Related Issues

  • Related Issue #
  • Closes #

@foodprocessor
Copy link
Collaborator Author

Now that errors are only returned in truly exceptional cases, settingsResponse is returned to the VMS basically every time that settingsReceived is called. That means we are sending an updated JSON schema to update the plugin settings UI every time we are called. That's good because it means we can update what the user sees on the plugin settings page. But it turns out our JSON is not always valid, and when it isn't, it crashes the whole server.
So I'm going to add a JSON validation step to settingsReceived to (hopefully) prevent ever returning malformed JSON to the VMS.

@foodprocessor
Copy link
Collaborator Author

Ok, all issues seem to be settled.
It took a lot of time to find out the hard way that fs::exists can throw an exception!

isMounted returning false does not guarantee that the drive letter has been removed.
@foodprocessor foodprocessor merged commit 9debd6c into main Aug 23, 2024
12 checks passed
@foodprocessor foodprocessor deleted the no-return-on-error branch August 23, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants