Add description to methods in build.js#44
Conversation
Added spec description to eth_getBlockByHash
Co-authored-by: Alita Moore
|
|
@timbeiko @MicahZoltu @vorot93 @lightclient @MariusVanDerWijden @draganrakita @gnidan @rekmarks Sooo would you all like me to mention you on each PR? |
|
You can exclude me 👍 |
|
Same, I'm not involved in day-to-day maintenance, although this does seem like a good PR! |
I don't think that is necessary. People interested can subscribe to this repository to get notifications for all PRs. Definitely mention me on things that may be particularly relevant to me though, as I don't know if I'll have the time to watch everything that flows through this repository. |
| descriptionFiles.forEach(file => { | ||
| let raw = fs.readFileSync(descriptionBase + file); | ||
| const methodName = file.split(".")[0] | ||
| let stringyDescription = raw.toString(); | ||
| methods.forEach((method, i) => { | ||
| if (method.name === methodName) { | ||
|
|
||
| methods[i].description = stringyDescription; | ||
| } | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Consider a for ... of loop here rather than a collection.forEach. Also recommend using const instead of let as it provides stronger guarantees to the reader and the compiler about intent.
| descriptionFiles.forEach(file => { | |
| let raw = fs.readFileSync(descriptionBase + file); | |
| const methodName = file.split(".")[0] | |
| let stringyDescription = raw.toString(); | |
| methods.forEach((method, i) => { | |
| if (method.name === methodName) { | |
| methods[i].description = stringyDescription; | |
| } | |
| }) | |
| }); | |
| for (const file of descriptionFiles) { | |
| const raw = fs.readFileSync(descriptionBase + file); | |
| const methodName = file.split(".")[0] | |
| const stringyDescription = raw.toString(); | |
| for (const method of methods) { | |
| if (method.name === methodName) { | |
| method.description = stringyDescription; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Though, a .map may actually be more appropriate here.
| ...methods, | ||
| ...parsed, | ||
| ]; | ||
| ...parsed |
There was a problem hiding this comment.
I think this change was a mistake? Most of this file is using tab for indentation, but some lines are using spaces. This should be fixed file/repository wide, but at least for this PR we probably shouldn't be making the situation worse. 😄
I recommend adding a .editorconfig to the root of the project so people's editors can automatically do the right thing with regards to indentation consistency.
There was a problem hiding this comment.
I'll add an .editorconfig. I feel relatively strongly that this should be a tab indented repo. There is very little alignment occurring and I think people should have the choice of how the view the specs. Of course, if there are good reasons against, I can be convinced otherwise....
There was a problem hiding this comment.
Okay, walking this back slightly. I think I'm okay for spaces in the js files as it appears to be the standard. We'll keep the spec files tab indented.
There was a problem hiding this comment.
Boo. Tabs are definitely superior here for the exact reason you mentioned.
@MicahZoltu ideally we want confirmation from all clients on each endpoint. And timing is important (we get paid by the hour), so if mentioning them would speed things up I'd champion for that. (I mentioned people before I saw this, btw) |
|
also cc @MicahZoltu I addressed your comments |
I would assume that we aren't paying you to sit and wait for PR approvals, if so we should probably reconsider the arrangement. 😉 I do think there is value in having one person from each client team actively watching this repository though, so we don't get stuck with PRs that are stale because we aren't getting the right eyes on them. |
|
@MicahZoltu @lightclient I am not sure if the edits to the build.js file are still needed, but I know that the rest of this PR is not needed anymore. If not I will close this PR |
|
I haven't been following this PR particularly closely. If you think a subset of it is worth review/merge then we should get the extraneous pieces removed so the bits that are useful can be reviewed in isolation. In general, many small PRs are preferable to one large PR anyway. 😄 |
No description provided.