Closed aborovsky closed 5 years ago
YoshiyukiKato
commented
5 years ago @aborovsky Hi! Thank you for your PR. I checked out this PR to my local and run test, but there are no error occurred...🤔 Do you have any other changes in your local? Or, It could be an environment problem. Could you please tell me your node and npm versions? (FYI, my node version is v11.6.0, and npm version is v6.7.0)
aborovsky
commented
5 years ago @YoshiyukiKato no changes compared to my forked master branch:
➜ grpc-mock git:(master) ✗ git remote -v
origin https://github.com/aborovsky/grpc-mock (fetch)
origin https://github.com/aborovsky/grpc-mock (push)
➜ grpc-mock git:(master) ✗ git status -sb
## master...origin/master
?? .idea/My environment:
➜ grpc-mock git:(master) ✗ node --version
v11.7.0
➜ grpc-mock git:(master) ✗ npm --version
6.7.0
➜ grpc-mock git:(master) ✗ python --version
Python 2.7.10
➜ grpc-mock git:(master) ✗ git --version
git version 2.17.2 (Apple Git-113)
➜ grpc-mock git:(master) ✗ uname -a
Darwin MacBook-Pro-boba.local 18.2.0 Darwin Kernel Version 18.2.0: Mon Nov 12 20:24:46 PST 2018; root:xnu-4903.231.4~2/RELEASE_X86_64 x86_64@aborovsky
Hmm...I cannot reproduce the error.
It might be a port problem. Please try to change the port of test server and client?
aborovsky
commented
5 years ago Ok, I’ll try
пт, 8 февр. 2019 г. в 20:35, Yoshiyuki Kato notifications@github.com:
@aborovsky https://github.com/aborovsky
Hmm...I cannot reproduce the error.
It might be a port problem. Please try to change the port of test server and client?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/YoshiyukiKato/grpc-mock/pull/6#issuecomment-461883082, or mute the thread https://github.com/notifications/unsubscribe-auth/ABl5LEYbpSWruuqesJx6WFQisB9FMGD5ks5vLbVGgaJpZM4audMR .
-- Отправлено с iPad.
aborovsky
commented
5 years ago @YoshiyukiKato tests adeed. We are good to go with review and merge current PR ;)
aborovsky
commented
5 years ago @YoshiyukiKato thanks a lot for great PR review! I've resolved all issues.
aborovsky
commented
5 years ago I think it is better not to use Error constructor, because it occurs difference of return type of the method between when _error has metadata and when it does not have.
GRPC’s frror is instance of Error. As far as i know. So it’s better to be compliant.
ср, 13 марта 2019 г. в 21:08, Yoshiyuki Kato notifications@github.com:
@YoshiyukiKato commented on this pull request.
@aborovsky https://github.com/aborovsky Thank you for updating! I wrote some comments about coding styles. Please check them out ;)
In index.js https://github.com/YoshiyukiKato/grpc-mock/pull/6#discussion_r265234529:
const m = new Metadata();
- Object.keys(error.metadata).forEach(key => m.add(key, String(metadata[key])));
- Object.keys(metadata).forEach(key => m.add(key, String(metadata[key])));
- [
- ...Object.keys(_error),
- "name", "message", "stack"
according to doc, grpc service error https://grpc.io/grpc/node/grpc.html#~ServiceError does not have name, message, and stack. Could you tell me why add these properties?
In index.js https://github.com/YoshiyukiKato/grpc-mock/pull/6#discussion_r265254830:
}
- const { metadata } = error;
- const error = new Error();
I think it is better not to use Error constructor, because it occurs difference of return type of the method between when _error has metadata and when it does not have.
In index.js https://github.com/YoshiyukiKato/grpc-mock/pull/6#discussion_r265259741:
const m = new Metadata();
- Object.keys(error.metadata).forEach(key => m.add(key, String(metadata[key])));
- Object.keys(metadata).forEach(key => m.add(key, String(metadata[key])));
- [
- ...Object.keys(_error),
- "name", "message", "stack"
- ]
- .map(fieldName => ({
- key: fieldName,
- value: _error[fieldName]
- }))
- .filter(({ key }) => (key !== "metadata" && _error.hasOwnProperty(key)))
- .forEach(({ key, value }) => error[key] = value);
I think prepareMetadata method is correct but redundant a little. It could be written more simply by using reduce method as follows:
const prepareMetadata = error => { if (!error.metadata) { return _error; }
const { metadata, code } = error; const grpcMetadata = Object.values(metadata).reduce((m, [k, v] => m.add(k, String(v)), new Metadata());
return { code, metadata: grpcMetadata } }
if you want to allow extra parameters, object spread syntax will help you.
const prepareMetadata = error => { if (!error.metadata) { return _error; }
const { metadata, code, ..._error } = error; const grpcMetadata = Object.values(metadata).reduce((m, [k, v] => m.add(k, String(v)), new Metadata());
return { code, metadata: grpcMetadata, ..._error } }
what do you think about these?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/YoshiyukiKato/grpc-mock/pull/6#pullrequestreview-214089633, or mute the thread https://github.com/notifications/unsubscribe-auth/ABl5LL-gMU4OJGLXJnJ1p8zBDF1Ck8eYks5vWT6sgaJpZM4audMR .
-- Отправлено с iPad.
aborovsky
commented
5 years ago @YoshiyukiKato please, review latest version
YoshiyukiKato
commented
5 years ago @aborovsky Thank you for updating. It looks great! I'm very happy to merge this PR. Thank you so much😆
Current PR can't be approved right now, cause there are no test coverage for new feature.
@YoshiyukiKato i need your help. I started to implement tests. But before any modification, i've tried to run current tests and they failed. What i'm doing wrong?
Tests run log: