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

Added notification when filename is wrong. #63

Merged
merged 30 commits into from
Jul 22, 2019
Merged

Conversation

FMounz
Copy link
Contributor

@FMounz FMounz commented Jul 19, 2019

Description:

This PR adds notification to the user when the file name being deployed or simulated is not main.py or code.py. We have that notification for the deployment, the simulation and the debugger.
A button is present on the simulator so it won't show another time during the session.
The deployment and debugging are cancelled in that case. The simulation will still happen.

Type of change

  • New feature (non-breaking change which adds functionality)

Limitations:

Please describe limitations of this PR

Testing:

  • Open a Python file which name is not code.py or main.py. An error message should appear when running the "deploy to device command", deployment should not happen.
  • Open a Python file which name is not code.py or main.py. An information message should appear when running the Pacifica debugger debugging continue. When clicking on the "Got it" or "Don't show again" button , the pop-up should close.
  • Open a Python file which name is not code.py or main.py. An information pop-up should appear when running the "run to device command". When clicking on the "Got it" or "Don't show again" button , the pop-up should close.
  • After clicking on "Don't show again" the message should not appear the next time.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

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.

Please look at suggestions. For the behavior of the debugger for the simulator, we want it such that we can still run .py files, but notify the user that if they want to run the same file on the device, they should rename it to code.py or main.py.

@FMounz
Copy link
Contributor Author

FMounz commented Jul 19, 2019

Please look at suggestions. For the behavior of the debugger for the simulator, we want it such that we can still run .py files, but notify the user that if they want to run the same file on the device, they should rename it to code.py or main.py.

Made some changes check them out.

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.

Looks alright to me aside from the capitalization of "python".

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.

@FMounz Some needed changes! but other than that looks good

Copy link
Contributor

@Christellah Christellah left a comment

Choose a reason for hiding this comment

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

Seems to work fine, just some suggestions/questions, specially reformatting would be nice :)

Copy link

@DIGarnier DIGarnier left a comment

Choose a reason for hiding this comment

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

Unsolicited advice 2: Electric boogaloo

src/extension.ts Outdated
...[
DialogResponses.DONT_SHOW,
DialogResponses.MESSAGE_UNDERSTOOD
]

Choose a reason for hiding this comment

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

Is there a reason why you're wrapping those enum values in an array and spreading them right after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean by spreading them right after?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

If you put something in an array and then use the spread operator ... you're taking the elements out of the array as if they we're never in one to begin with!

look what happens here if you press play

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get you now after reading this .
Thanks for that very enlighting

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, one comment

}else if(!validCodeFileName(currentFilePath) && shouldShowInvalidFileNamePopup){
vscode.window
.showInformationMessage(CONSTANTS.INFO.INVALID_FILE_NAME_DEBUG,
...[
Copy link
Member

Choose a reason for hiding this comment

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

if we're removing the spread operator elsewhere might as well do it here too?

@FMounz FMounz merged commit 81b57ee into dev Jul 22, 2019
@Christellah Christellah deleted the users/t-chcido/filename-check 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.

5 participants