Remove setup from `LintRule` to ease the writing of rules

[samuelstroschein]

Problem

Creating lint rules involves two setup functions, leading to boilerplate for every lint rule.

Every lint rule in standard-lint-rules involves the pattern showcased below.

Proposal

1. Remove setup from from LintRule
2. Extend Context with referenceLanguage and languages.
3. Pass Context to visitors.

export const additionalKeyRule: LintRuleInitializer = createLintRule(
    'inlang.additionalKey',
    'warn',
    () => {
-       let context: Context
-       let referenceLanguage: string

        return {
-           setup: (args) => {
-               context = args.context
-               referenceLanguage = args.referenceLanguage
-           },
            visitors: {
                                 // pass context as part of visitors
                Resource: ({ target, context }) => {
                    if (target && target.languageTag.name === context.referenceLanguage) return 'skip'
                },
                                 // pass context as part of visitors
                Message: ({ target, reference, context }) => {
                    if (!reference && target) {
                        context.report({
                            node: target,
                            message: `Message with id '${target.id.name}' is specified, but missing in the reference`
                        })
                    }
                },
            },
        }
    })

[samuelstroschein]

@ivanhofer The idea behind setup function 2 was to make external API requests. However, the createLintRule utility effectively became a setup function that can make fetch requests and store additional information.

Setup function 2 became redundant. Taking the example from the README and refactoring it:

[samuelstroschein]

On that note, the teardown function is likely also redundant. Closing a connection to a service implies a stream that needs to be closed. Chances are that streams won't work in many environments like CI/CD and 99.99% of use cases will make REST requests.

No standard lint rule uses teardown. I vote for removing teardown. If users request it, we can re-add it.

If both setup and teardown are removed, the visitor's property could be unbundled to further increase the DX of writing rules. The left code shows a rule without setup and visitors compared to the current implementation on the right.

[ivanhofer]

The separation between those two "setup" function is that the first one can't be async because it get's called inside the inlang.config.js. It also does not know anything about languages as this is only known once the inlang.config.js file get's executed.

An async setup function can be needed sometimes if you need to authenticate to a server e.g. retrieve a token, that will be used for all other API calls. Sure you can do that also inside visitors but it does not look clean to me.

The teardown functionality could also be used for reporting purposes. Not sure if someone needs it, but supporting it is not a bad idea in my opinion.

We could make setup optional and pass those arguments to each visitor. Then we can get rid of those few lines if we don't really need the setup function.

[samuelstroschein]

The separation between those two "setup" function is that the first one can't be async because it get's called inside the inlang.config.js. An async setup function can be needed sometimes if you need to authenticate to a server e.g. retrieve a token

We can make lint rules async. The defineConfig file under which lint rules are executed is async.

But, connecting to an external service almost always authorizes with the network request e.g. fetch. Prior "setup" is not required.

Not sure if someone needs it, but supporting it is not a bad idea in my opinion. It also does not know anything about languages as this is only known once the inlang.config.js file get's executed.

That's what I fear. We don't know what people will use/want to use. Lint seems overly great in complexity to accustom maybe future use cases. I propose that we try to minimize all external facing API and wait for users to demand features before we expose maybe nice-to-have features.

Proposal

• remove setup and teardown
• leave createLintRule sync until users demand an async createLintRule function

Questions

• Leave visitors as is even if setup and teardown are removed?
• Context could include target, reference, referenceLanguage, languages, and report to simplify the explanability of creating a lint rule. "Every visitor callback receives a context object that includes [...]"

PS I am guilty. We both worked on lint. It is only now that I try to use it myself that I realize my feedback was flawed.

[ivanhofer]

We can make lint rules async. The defineConfig file under which lint rules are executed is async.

But then the vs code extension would need to wait until the lint rule, that is deactivated did it's setup.

I would keep two different "initialize" and "setup" functions.

Try to implement a "grammar check" rule and see what you need.

[samuelstroschein]

I would keep two different "initialize" and "setup" functions.

We can always re-introduce setup when users demand it, but we can't remove setup in the future without a breaking change.

Try to implement a "grammar check" rule and see what you need.

What do you mean?

[ivanhofer]

but we can't remove setup in the future without a breaking change

If we can't remove it then I would assume the demand was there ^^

What do you mean?

Create a new lint rule that talks to an API and see what concepts you need. The current rules only need the basics. I don't think that all rules will be implemented in < 5 lines of actual code

[samuelstroschein]

Create a new lint rule that talks to an API and see what concepts you need.

@ivanhofer I went ahead and created a mock rule that queries ChatGPT.

Observations

1. We can't use secrets for APIs 🙃😆

Inlang has no concept of env variables yet. Using process.env or similar won't work. Thus, the idea of opening a connection with an external service that requires a secret won't fly (for now).

2. Even if APIs are called the double setup function is redundant.

The only need for the second setup function is to store the context in the first setup function. API SDKs are not initialized in neither setup function.

Source code

```ts import { Context, createLintRule } from "@inlang/core/lint"; import { Configuration, OpenAIApi } from "openai"; const openai = new OpenAIApi( new Configuration({ apiKey: "123456789101234556", }) ); const rule = createLintRule("hello.world", "error", () => { let context: Context; return { setup: (args) => { context = args.context; }, visitors: { Message: async (args) => { const result = await openai.createChatCompletion({ model: "ChatGPT3.5", messages: [ { content: JSON.stringify(args.target!.value), role: "user" }, ], }); if (result.status !== 200) { context.report({ message: result.data.choices[0].message!.content, node: args.target!, }); } }, }, }; }); ``` The code can be simplified by factoring out the API function ```ts import { Context, createLintRule } from "@inlang/core/lint"; import { Configuration, OpenAIApi } from "openai"; const openai = new OpenAIApi( new Configuration({ apiKey: "123456789101234556", }) ); const rule = createLintRule("hello.world", "error", () => { let context: Context; return { setup: (args) => { context = args.context; }, visitors: { Message: async (args) => { const [isError, message] = await queryChatGpt(args); if (isError) { context.report({ message, node: args.target!, }); } }, }, }; }); async function queryChatGpt(args: any): Promise<[boolean, string]> { const result = await openai.createChatCompletion({ model: "ChatGPT3.5", messages: [{ content: JSON.stringify(args.target!.value), role: "user" }], }); const isError = result.status !== 200; return [isError, result.data.choices[0].message!.content!]; } ```

Proposal

The goals of the lint feature where unaligned. I am to blame. The current version tries to solve many edge cases. In this proposal, I am trying reduce complexity, expose less features, and wait for user feedback on edge cases before we expose features that might not align with user needs but need to be maintained by us.

• remove setup and teardown properties
• do not expose the concept of on enter or leave [and wait for user feedback to re-introduce the feature]
• leave createLintRule sync until users demand an async createLintRule function

Questions

• Context could include target, reference, referenceLanguage, languages, and report to simplify the explanability of creating a lint rule. "Every visitor callback receives a context object that includes [...]"

[ivanhofer]

1. We can't use secrets for APIs

Maybe we need to bring back the environment functions ^^

In this proposal, I am trying reduce complexity, expose less features, and wait for user feedback on edge cases before we expose features that might not align with user needs but need to be maintained by us.

Ok, let's do that.

• Context could include target, reference, referenceLanguage, languages, and report to simplify the explanability of creating a lint rule

Yes each visitor should receive all params.

• Leave visitors as is even if setup and teardown are removed?

I would keep the extra level to be able to extend it in the future

[samuelstroschein]

Alright, I'll get to work. Hopefully done by tonight.

Maybe we need to bring back the environment functions ^^

Haha. Most likely related to sandboxing the JS. If we manage to sandbox JS, the environment functions become redundant.

I would keep the extra level to be able to extend it in the future

Will keep the nested layer.

[samuelstroschein]

closed by #459