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

Make possible to upgrade packages within IDE #1467

Merged
merged 2 commits into from
Oct 15, 2018

Conversation

brusherru
Copy link
Contributor

It closes #1453.

@brusherru brusherru self-assigned this Oct 12, 2018
@brusherru brusherru requested a review from a team October 12, 2018 17:45
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Suggest fixing the terminology in user messages and JS symbol names. When we download and install, it is an upgrade. When we check, we are checking for updates. That is, upgrade is an action: desctructive and not undoable; update is informational it is a noun.


renderCores() {
if (!this.state.cores) {
return <span>Checking for updates. Please wait...</span>;
Copy link
Member

Choose a reason for hiding this comment

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

Just: “Checking for updates...”


return (
<div>
<p>There are some packages you can update:</p>
Copy link
Member

Choose a reason for hiding this comment

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

Just “Packages to upgrade”

@@ -0,0 +1,3 @@
export const SHOW_UPDATE_ARDUPACKAGES_POPUP = 'SHOW_UPDATE_ARDUPACKAGES_POPUP';
export const HIDE_UPDATE_ARDUPACKAGES_POPUP = 'HIDE_UPDATE_ARDUPACKAGES_POPUP';
export const RUN_UPDATE_PROCESS = 'RUN_UPDATE_PROCESS';
Copy link
Member

Choose a reason for hiding this comment

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

Eh... Flux misuse. Actions are facts, not intents. That is:

  1. ARDUPACKAGES_UPDATE_REQUESTED — it is our decision should any dialog be shown or two, or the upgrade should start right away
  2. POPUP_DISCARDED — it does not matter which popup, its identifier might be included in the payload in a unified way. The fact is just about clicking the cross icon, “Cancel” button, or ESC key
  3. ARDUPACKAGES_UPGRADE_PROCEED — the reaction is the same, but semantics is better

Copy link
Member

Choose a reason for hiding this comment

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

After googling again for Redux action name conventions, I think the best one to stick to is <NOUN>_<VERB_INFINITIVE>. That is:

  • ARDUPACKAGES_UPDATE_REQUEST
  • POPUP_DISCARD or POPUP_CLOSE
  • ARDUPACKAGES_UPGRADE_PROCEED

See

@@ -106,6 +106,9 @@ const rawItems = {
uploadToArduino: {
label: 'Upload to Arduino...',
},
updatePackages: {
label: 'Update Packages...',
Copy link
Member

Choose a reason for hiding this comment

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

Upgrade Arduino Packages & Toolchains...

@brusherru brusherru force-pushed the feat-1453-upgrade-packages branch 2 times, most recently from f3ac38a to 8f199ab Compare October 15, 2018 10:40
@brusherru
Copy link
Contributor Author

@nkrkv fixed but just rename HIDE_UPDATE_ARDUPACKAGES_POPUP to UPDATE_ARDUPACKAGES_POPUP_CLOSED

*
* :: (Object -> _) -> ArduinoCli -> Promise String Error
*/
const updateArduinoPackages = (onProgress, cli) => cli.core.upgrade(onProgress);
Copy link
Member

Choose a reason for hiding this comment

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

upgrade

subscribeIpc(() => checkUpdates(cli), CHECK_ARDUINO_DEPENDENCY_UPDATES);
};

export const subscribeUpdateArduinoPackages = cli => {
Copy link
Member

Choose a reason for hiding this comment

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

upgrade?

client.deleteProcess(processId, client.INSTALL_ARDUINO_DEPENDENCIES)
);
export const showUpdatePackagesPopup = () => ({
type: AT.ARDUPACKAGES_UPDATE_REQUESTED,
Copy link
Member

Choose a reason for hiding this comment

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

Name after action type:

updateArdupackages or updatePackages

type: AT.ARDUPACKAGES_UPDATE_REQUESTED,
});
export const hideUpdatePackagesPopup = () => ({
type: AT.UPDATE_ARDUPACKAGES_POPUP_CLOSED,
Copy link
Member

Choose a reason for hiding this comment

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

closePackageUpdatePopup

@@ -41,3 +41,37 @@ export const deleteProcess = (id, type) => ({
payload: { id },
meta: { status: STATUS.DELETED },
});

export const createProcess = type => dispatch => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Uploading to Nano with newer bootloader
3 participants