Skip to content

Remove constraint making isStatic required - #2174

Merged
0candy merged 1 commit into
masterfrom
change_remote_method
Mar 31, 2016
Merged

Remove constraint making isStatic required#2174
0candy merged 1 commit into
masterfrom
change_remote_method

Conversation

@0candy

Copy link
Copy Markdown
Contributor

Comment threadlib/registry.js Outdated
key = isStatic ? key : m[1];
meta.isStatic = isStatic;
} else {
if (meta.isStatic && m) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nitpick: the new block {...} starting on L268 is not necessary, it adds extraneous nesting. Here is a version that I consider cleaner:

if(typeofmeta.isStatic!=='boolean'){// ...}elseif(meta.isStatic&&m){// ...}else{// ...}

@bajtos

Copy link
Copy Markdown
Member

@0candy one more thing, please add an entry to 3.0-RELEASE-NOTES.

@bajtosbajtos assigned 0candy and unassigned bajtosMar 29, 2016
@0candy0candy assigned bajtos and unassigned 0candyMar 30, 2016
Comment thread3.0-RELEASE-NOTES.md Outdated
'prototype.instanceMethod': {
http: { path: '/instance' }
}
``` No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a link to this pull request, to make it easier for readers to look up discussions that led to this change. AFAICT, other entries include a link to the relevant pull request too.

@bajtos

Copy link
Copy Markdown
Member

@0candy one last comment to address. When you fix it, please squash the commits and land the patch, no need for more reviews.

Some of the CI builds are failing, please check that these failures are not caused by your change.

@bajtosbajtos assigned 0candy and unassigned bajtosMar 30, 2016
@bajtos

Copy link
Copy Markdown
Member

The Travis build is failing because we haven't released a new juggler version with loopbackio/loopback-datasource-juggler@0d11186. We are withholding 2.x releases until @Amir-61 finishes his work on replaceOrCreate feature, to ensure we won't need to change the implementation that is already there in a backwards-incompatible way.

@0candy
0candyforce-pushed the change_remote_method branch 2 times, most recently from 7bba791 to 6157a3dCompareMarch 30, 2016 15:14
@0candy
0candyforce-pushed the change_remote_method branch from 6157a3d to b5b900eCompareMarch 30, 2016 15:17
@0candy
0candy merged commit 982b8ac into masterMar 31, 2016
@0candy
0candy deleted the change_remote_method branch March 31, 2016 13:59
@0candy0candy removed the #review label Mar 31, 2016
@lehni

Copy link
Copy Markdown
Contributor

@0candy, @bajtos please note that this change has cause a new issue, see #3529

Sign up for freeto 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.

3 participants

@0candy@bajtos@lehni