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

PB-435 Rethink migration #5

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

tortillaj
Copy link

No description provided.

table.text('description');
table.string('name');
table.string('plugin');
// @todo: do we want this to be an enum, or leave it open to more statuses?
Copy link
Author

Choose a reason for hiding this comment

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

My inclination is to just leave it open so that new plugins created in the main probo project don't also need a sibling PR in this project too.

lib/Api.js Outdated
@@ -225,6 +225,23 @@ class Api {
});
}

updateObject(req, res, next) {
let self = this;
Copy link
Author

Choose a reason for hiding this comment

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

@kleinmp @dzink - Your default variable declaration should be const instead of let. In JS, const is still bound by function scope (really any block scope - e.g. var is not bound by if statements but const and let are), it can't leak out of this scope. It does not mean the same thing in JS as it does in PHP or other languages.

The reason to use const by default is to ensure the variable binding remains the same. let can be redefined within a scope, whereas const cannot. Defaulting to const requires you to think about if you want this variable to be reboundable to something different. 95% of the time, you never want to reuse the same variable name within one scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants