Skip to content
This repository has been archived by the owner on Dec 23, 2021. It is now read-only.

Improvements from bug bash comments #56

Merged
merged 6 commits into from
Jul 18, 2019

Conversation

Christellah
Copy link
Contributor

@Christellah Christellah commented Jul 17, 2019

Description:

In this PR, we have the following improvements :

  • Reset the switch state on the SVG to off when we run the code
  • Indicate the name of the file selected to be deployed on the device or the simulator
  • Prevent the Python scripts process_user_code.py and device.py to run if no file has been selected before
  • Change the wording of the code deployment to device message to avoid confusion
  • Kill the Python process running when the webview is closed

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Limitations:

The file name checking is not part of this PR, we still allow all names on simulator and device

Testing:

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Try to run a code -> set the switch to on -> run a code => switch's state should be cleared and back to off
  • Try to run a file and check if the name is correctly printed in the output console with deployment to the device or the simulator
  • Try deploying code before selecting any file => no other errors should show up after "Can't find a .py file" because the scripts are not running
  • Read the code deployment to device successful message
  • Try to run a code -> close the webview -> open the simulator webview => no code should be running

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

@LukeSlev
Copy link
Member

Think you forgot about the killing of the process on webview close 😅

Copy link
Member

@LukeSlev LukeSlev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@markAtMicrosoft
Copy link
Contributor

markAtMicrosoft commented Jul 18, 2019

Why do we have a single PR to do so many things? We need to limit PRs to doing one thing, then move on.

Copy link
Contributor

@jonathanwangg jonathanwangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected. Nice changes that'll be helpful for the user. Only suggestion would be to either move the runSimulator() or break it down into smaller methods since it's getting quite long with nested if and switch statements.

@Christellah
Copy link
Contributor Author

Works as expected. Nice changes that'll be helpful for the user. Only suggestion would be to either move the runSimulator() or break it down into smaller methods since it's getting quite long with nested if and switch statements.

To avoid adding more things in that PR I think we should have a separate PR for refactoring the run method

@Christellah Christellah merged commit 67357da into dev Jul 18, 2019
@Christellah Christellah deleted the users/t-chcido/bugbash-improvements branch August 7, 2019 18:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants