BaseService: Factor out a couple methods #1425
Conversation
- Adjust test grouping - Rename data -> queryParams, text -> message
Generated by 🚫 dangerJS |
|
@platan @RedSparr0w Would you mind giving this a read? |
|
@paulmelnikow I will look at this tomorrow. |
| if (serviceData.text) { | ||
| text[1] = serviceData.text; | ||
| if (serviceData.message) { | ||
| text[1] = serviceData.message; | ||
| } | ||
| Object.assign(badgeData, serviceData); |
There was a problem hiding this comment.
message is copied from serviceData to badgeData here. Do we need it in badgeData?
There was a problem hiding this comment.
Nice catch, I don't think it is currently used in badgeData.
Also not sure if there maybe a reason why it was this way, but could we also change the following to directly assign the value:
- const text = badgeData.text;
- if (serviceData.text) {
- text[1] = serviceData.text;
+ if (serviceData.message) {
+ badgeData.text[1] = serviceData.message;
}
- badgeData.text = text;Same thing with line 116-118.
Seems to be ~30-50% slower when re-assigning with an array.
There was a problem hiding this comment.
We don't need message in badgeData.
I think the reason the assignment was done strangely is that text was being copied from serviceData into badgeData. Since it's not called text anymore, that shouldn't matter.
Maybe the least surprising thing here would be to whitelist the acceptable keys from serviceData, and copy only what is present.
There was a problem hiding this comment.
I rewrote this to make everything more explicit, but I think I will defer it to the next PR because the diff is again pretty large.
| static register(camp, handleRequest) { | ||
| const serviceClass = this; // In a static context, "this" is the class. | ||
|
|
||
| static get _regex() { |
There was a problem hiding this comment.
Good idea to extract _regex and _namedParamsForMatch 👍.
| const captures = match.slice(1, -1); | ||
|
|
||
| if (this.uri.capture.length !== captures.length) { | ||
| throw new Error( |
There was a problem hiding this comment.
I do not know all the plans for services rewrite but in my opinion it would be good idea to add test for this fragment and for other untested code in this base class.
There was a problem hiding this comment.
Agreed this is a great idea.
|
I looks good to me. The base class looks better now 😄 . I've left one question. |
…1543 This merges the `node-8` branch. The heavy lift was by @Daniel15 with refactoring from me and a patch by @RedSparr0w. * New API for registering services (#963) * Disable Node 6 tests on node-8 branch (#1423) * BaseService: Factor out methods _regex and _namedParamsForMatch (#1425) - Adjust test grouping - Rename data -> queryParams, text -> message * BaseService tests: Use Chai (#1450) * BaseService: make serviceData and badgeData explicit and declarative (#1451) * fix isValidStyle test (#1544) * Run tests in Node 9, not Node 6 (#1543)
In the process of adding and testing some error-handling code to the base services, I factored out a couple methods. The diff is a bit gnarly. I think those changes will be easier to read if this is reviewed separately.