Add lowercase directives

richardm90 commented 8 months ago

Changes

There is a linter rule for uppercase directives but not for lowercase directives, I like my directives to be in lowercase, so I've added a new LowercaseDirectives rule.

Whilst making this change I also corrected a couple of issues.

Fix IncorrectVariableCase when definition exists in copybook, only noticed when using the Format Document option
Fix Quick Fix comment for RequireBlankSpecial
Added tests for IncorrectVariableCase, UppercaseDirectives, LowercaseDirectives

Questions/Points

Would it be better to have a single rule for the directive case such as DirectivesCase: ['upper', lower']? This would deprecate the existing UppercaseDirectives rule.
Is there a way to actually run Quick Fixes as part of unit tests? One of the fixes I implemented only occurred when the Format Document option was run.
I'll update the docs once I know the pull request is acceptable.
I initially tried using npm run test to run the test suite but this failed with a module not found error. I'm pretty sure this is because the typescript needs to be compiled before the tests are run but I couldn't work out how this be done so used the 'Debug Tests' from the VS Code debugger instead.

Checklist

[x] have tested my change
[ ] updated relevant documentation
[x] Remove any/all console.logs I added
[x] eslint is not complaining
[x] have added myself to the contributors' list in the README

worksofliam commented 7 months ago

Would it be better to have a single rule for the directive case such as DirectivesCase: ['upper', lower']? This would deprecate the existing UppercaseDirectives rule.

I absolutely think is the way to do (DirectivesCase ✅) it instead of having two separate rules (UppercaseDirectives, LowercaseDirectives 👎). Don't remove UppercaseDirectives from the schema definition (rpglint.json), but instead mark it as deprecated: true.

Is there a way to actually run Quick Fixes as part of unit tests?

Quick Fixes, no, sadly not. But we can test what location of where the fix will take place. See this assert for example:

  assert.deepStrictEqual(errors[0], {
    offset: { position: 194, end: 202 },
    type: `IncorrectVariableCase`,
    newValue: `localVar`
  });

It's ensuring the ranges in the document, the error type and what the new value will be when the user quick fixes it. This has been a very suitable test so far. I see you are already doing this!

... but this failed with a module not found error.

This is very odd! I am glad the Debug Tests worked from VS Code. I would recommend ensuring you used npm i before running npm run test again. Feel free to share the logs with me.

Let me know what else you need from me for this PR.

richardm90 commented 7 months ago

@worksofliam, I've just started work on these changes but wanted to clarify one of yours points.

I absolutely think is the way to do (DirectivesCase ✅) it instead of having two separate rules (UppercaseDirectives, LowercaseDirectives 👎). Don't remove UppercaseDirectives from the schema definition (rpglint.json), but instead mark it as deprecated: true.

deprecated isn't a valid property in rpglint.json. Shall I add it anyway? I couldn't find any other examples in the code base except for language/models/tags.js.

worksofliam commented 7 months ago

@richardm90 add for now. This keyword was added back in 2019 to json schema.

https://json-schema.org/draft/2019-09/json-schema-validation#rfc.section.9.3

richardm90 commented 7 months ago

@richardm90 add for now. This keyword was added back in 2019 to json schema.

https://json-schema.org/draft/2019-09/json-schema-validation#rfc.section.9.3

It looks as though the new JSON schema version isn't fully supported in VS Code yet but I've added the property.

[json] JSONSchema draft 2019-09 support #98724

richardm90 commented 7 months ago

... but this failed with a module not found error.

This is very odd! I am glad the Debug Tests worked from VS Code. I would recommend ensuring you used npm i before running npm run test again. Feel free to share the logs with me.

I tried npm i but still no joy. This is the output in the terminal. I think it might be something to do with Typescript transpiling.

$ npm run test

> vscode-rpgle@0.24.0 test
> tsx tests

(node:29973) ExperimentalWarning: `--experimental-loader` may be removed in the future; instead use `register()`:
--import 'data:text/javascript,import { register } from "node:module"; import { pathToFileURL } from "node:url"; register("file%3A///home/richard/git/vscode-rpgle/node_modules/tsx/dist/loader.js", pathToFileURL("./"));'
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/modules/cjs/loader:1134
  const err = new Error(message);
              ^

Error: Cannot find module '../parserSetup'
Require stack:
- /home/richard/git/vscode-rpgle/tests/suite/basics.js
- /home/richard/git/vscode-rpgle/tests/suite/index.js
- /home/richard/git/vscode-rpgle/tests/index.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1134:15)
    at Module._load (node:internal/modules/cjs/loader:975:27)
    at Module.require (node:internal/modules/cjs/loader:1225:19)
    at require (node:internal/modules/helpers:177:18)
    at Object.<anonymous> (/home/richard/git/vscode-rpgle/tests/suite/basics.js:5:34)
    at Module._compile (node:internal/modules/cjs/loader:1356:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1414:10)
    at Module.load (node:internal/modules/cjs/loader:1197:32)
    at Module._load (node:internal/modules/cjs/loader:1013:12)
    at Module.require (node:internal/modules/cjs/loader:1225:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/richard/git/vscode-rpgle/tests/suite/basics.js',
    '/home/richard/git/vscode-rpgle/tests/suite/index.js',
    '/home/richard/git/vscode-rpgle/tests/index.js'
  ]
}

Node.js v18.19.0

worksofliam commented 7 months ago

@richardm90 Second ask: please update the docs with a PR over there when you can!

richardm90 commented 7 months ago

@worksofliam Ah OK. I wasn't clear on deprecated stuff. I thought there would be a period of time before the feature was removed from the codebase. I'm happy to strip it out of the codebase though and presumably remove if from the docs as well?

worksofliam commented 7 months ago

@richardm90 leave it in the docs for a while but leave a note that it's deprecated!

codefori / vscode-rpgle