Disable remote method by alias #385
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
|
Can one of the admins verify this patch? |
|
@Overdrivr : Thank you for your contribution. |
Overdrivr
force-pushed
the
disable-remote-by-alias
branch
from
0023e01 to
464cc8b
Compare
|
@gunjpan Rebase is done |
|
@slnode ok to test |
|
I'm a bit confused with the failing test reports, however they don't seem to be required. Is there something I need to do to make them pass ? |
|
@Overdrivr : I am going to rerun it. We've removed one component from |
|
@slnode test please |
| @@ -295,6 +295,70 @@ describe('SharedClass', function() { | |||
| expect(methods).to.not.contain(INST_METHOD_NAME); | |||
| expect(methods).to.not.contain('instTestMethod'); | |||
| }); | |||
|
|
|||
| it('excludes disabled prototype methods from the method list using ' + | |||
There was a problem hiding this comment.
Please reword this and others (use your best judgement) to make it easy:
excludes prototype methods from method list disabled by alias
| it('excludes disabled prototype methods from the method list using ' + | ||
| 'a method alias', function() { | ||
| var INST_METHOD_NAME = 'instTestMethod'; | ||
| var INST_METHOD_ALIAS = 'instTestMethodAlias'; |
There was a problem hiding this comment.
while we're here, please extract these variables out after L#80 and put the initialization in beforeEach() block (same as L#251) so that there is no repetition of same code.
|
Thank you for the feedback, let me know if the refactoring looks ok |
There was a problem hiding this comment.
Changes LGTM.
Please rebase commits on master and squash them to one commit. Also, please provide small description of what it is trying to fix. See commit guideline.
Overdrivr
force-pushed
the
disable-remote-by-alias
branch
from
e1313ea to
0f21ffb
Compare
SharedClass.disableMethodByName now checks all method aliases in addition to the method name, for disabling a method
Overdrivr
force-pushed
the
disable-remote-by-alias
branch
from
0f21ffb to
5ac1161
Compare
|
@slnode test please |
|
@Overdrivr : Landed. @bajtos : I believe we need to backport this, thoughts? |
👍 Please review the diff in 2.x carefully, we have made some changes in the way how remote methods are looked up in 3.0 and I am not sure if the backwards-compatible API changes were back-ported to 2.x. |
|
@Overdrivr : Could you please back-port this change against [email protected] Should you need any assistance, please let me know. Thank you for your contributions :) ! |
|
Sure, just to be clear I need to make a PR against this branch, after I checked everything is running ok ? |
|
@gunjpan I tried rebasing the branch from |
|
Yes, back-porting sometimes is not straight forward and you may need to integrate code changes manually. Not sure how much of that will require for this PR though. Please give it a try and see how much effort is that. If it is too big of a change, we'll have to be considerate on what we backport. I may give it a try when I find some time. Thank you for your efforts :) |
For strongloop/loopback#1936