Follow standard style#719
Conversation
|
Of course, following a standard coding style in the docs is a fine idea, as is enforcing it using a linter. I assume (based on expressjs/discussions#18) that the omission of semicolons at end of statements is from http://standardjs.com/. I didn't realize that we had "officially" adopted that as the standard (although it was discussed in that issue and in the TC--forgive me if I'm just forgetting that conclusion!) I have no objection, but I notice that the actual code in the repo and the examples does not follow that convention--in fact both places seem to consistently use semicolons. I didn't investigate other stylistic conventions. Regardless of the style guide we adopt, I think we can all agree that the actual code & examples should match the docs. So, would it make sense to also open a PR for those changes and commit it at the same time? If that's in the works, great. I just think we should keep it all in sync, rather than doing it piecemeal and having them be different for an indeterminate period. Not to broaden the scope of this too much, but we should also address expressjs/express#3078, that is clearly state what the coding style guide we're using. Currently, it is not clear, unless you spelunk in the various related issues. |
|
@dougwilson Do you think we should go ahead and merge this or wait for the style to be applied to examples & code first? I know we discussed this in a TC meeting, but don't recall if there was a firm decision. I'm good either way.... |
|
Basically, at your discretion. |
| ``` | ||
|
|
||
| You define error-handling middleware last, after other `app.use()` and routes calls; for example: | ||
| You define error-handling middleware last, after other `app.use()` and routes calls for example: |
There was a problem hiding this comment.
Please add back the semicolon after "calls".
There was a problem hiding this comment.
Whooops, sorry about that, will fix 👌
| ``` | ||
|
|
||
| Also in this example, `clientErrorHandler` is defined as follows; in this case, the error is explicitly passed along to the next one. | ||
| Also in this example, `clientErrorHandler` is defined as follows in this case, the error is explicitly passed along to the next one. |
There was a problem hiding this comment.
Please add back the semicolon after "as follows".
|
|
||
| If you pass an error to `next()` and you do not handle it in | ||
| an error handler, it will be handled by the built-in error handler; the error will be written to the client with the | ||
| an error handler, it will be handled by the built-in error handler the error will be written to the client with the |
There was a problem hiding this comment.
Please add back the semicolon after "built-in error handler".
|
@LinusU Sorry for the delay merging this. I think if you address my comments, we should be able to merge. |
|
No problem, I'll fix them asap 👍 |
|
Done 👌 |
Sorry for me never responding to your first message :) |
|
OK, LGTM. One thing I should mention is that the files in the So, we can merge this, but then the next time we run the script, these changes will get clobbered. To fix this, we'd need to commit the changes back to the README in the middleware repos. |
|
BTW, this reminds me that I need to document the whole README process/script.... Bad me. |
|
We'll need to fix the example broken in expressjs/express#3120 and probably look through the PR more to see if anything else may have been broken. |
|
Sorry about that. I guess I should have looked through the changes to the middleware READMEs more closely. I'm inclined to just rerun the README script and recommit the READMEs, which would undo all the changes to the middleware READMEs made in this PR. But then the code style in the READMEs (use of Thoughts? |
|
I went thru the commit again and looked extra hard at every |
|
Oh, great @LinusU so you think it's all good at this point? If so, then the next step would be to take the README files and open PRs to change the READMEs in the middleware repos accordingly. If you have time, that would be great. It should be as simple as checking out all the middleware repos, copying the markdown portion of the README files into those repos then opening PRs. Not difficult, but a bit time-consuming. LMK if you have questions about this. I can also help with this part. |
|
Looks like @dougwilson beat me to it 😄 Are you doing all repositories @dougwilson, or was it just a coincidence that we picked the same one at the exact same time? |
|
Oh and yeah, I think that it's all good at this point |
|
I was going to do a few. Should have posted here :) I was just playing with one today, haha, so your PRs are great 👍 |
|
Hehe, what do you think about adding the eslinting step directly in those repositories? That way we don't have to wait for a sync here to see that they are "broken"... Although it might conflict with using |
|
Need to get some sleep so won't do anymore tonight, I'll be continuing later in the week :) |
3 participants
This adds eslint with together with a config + plugins for linting javascript inside of markdown files.
Currently only the
en/directory is fixed and being checked by the linter, but it would be easy to fix the other languages with additional pull requests.