Skip to content

Issue/165 #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: issue/165
Choose a base branch
from
Open

Issue/165 #1

wants to merge 13 commits into from

Conversation

ivanitskiy
Copy link

Make sure VSC can identify njs-types and added eslint, prettier and misc changes

dekobon and others added 4 commits August 16, 2023 17:09
This change adds support for building JSDoc docs
using nodejs. It also improves the JSDoc annotations
used in the njs source code.
- prettier to autoformat JS code
- eslint to lint code
- jsconfig.json to make VSC happy and able to discover njs-types
- moved jsdoc/conf.json to .jsdocsrc.json
@ivanitskiy
Copy link
Author

VSC still reports some issues, but eslint doesn't report them:-(
image

need to use the same njs-types as it is used NJS_VERSION to
properly lint agains the right version
- r.variables contains only strings, so r.variables.cache_instance_credentials_enabled == 1 may cause issues
- updated typedef S3ReqParams and make properties optional
- NjsStringOrBuffer has no indexOf, so explicitly invoke toString()
- import Credentials typedefs where missed
@ivanitskiy
Copy link
Author

addressed most of the issues reported in the previous screenshot via the last two commits (d6fb9b0 and 44d21d4)

Copy link
Collaborator

@4141done 4141done left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we are putting the jsdoc config into a more standard place with this change.

Although we're not adding anything to the merge path, I am still of the opinion that eslint and prettier should both be separate PRs for three reasons:

  1. A general practice of keeping PRs about one thing.
  2. Allowing focused discussion on choices for the configuration of each tool by us and the community.
  3. In my experience, formatting is better introduced along with a CI gate on formatting for merge. Otherwise we're constantly reminding people to format and diffs get confusing.

@ivanitskiy
Copy link
Author

well when semi: true: VSC code complains about it:
image

@4141done
Copy link
Collaborator

well when semi: true: VSC code complains about it: image

That smells like an issue with your editor config. Strange that it would be flagging semicolons

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants