refactor: use minimum config schema, remove env from lint

[samuelstroschein]

Problem

I went through the source code of lint and found numerous things we should likely change. I am to blame as I reviewed and accepted the PR that introduced the issues beneath.

1. Linting depends on the entire inlang config even though only languages, referenceLanguage, and lint is used.
2. Linting depends on the env and makes readResources calls under the hood.
3. Arguments of functions do not use the object pattern, making public API changes difficult.
4. The surface area, especially of query, seems too large.

Proposal

1. Only use the bare minimum of the config schema

All modules in core are supposed to be used individually as much as possible. By forcing the existence of config properties that are unused like writeResources, the goal is contradicted. Furthermore, testing becomes easier because redundant properties do not need to be mocked.

// current
export const lint = async (config: Config, env: EnvironmentFunctions )

// proposal
export const lint = async (
  config: Pick<Config, "lint" | "languages" | "referenceLanguage">, 
  resources: ast.Resource[], 
  env: EnvironmentFunctions 
)

2. Remove the environment function from lint

• Exposing the environment functions increases the complexity of lint by a wide margin. A lint rule should not depend on the file system or dynamically importing JavaScript. If the latter use case emerges, we can introduce environment functions again.
• Testing complexity increases because the env needs to be passed around.
• No tests and rules in standard-lint-rules use environment functions.

// current
export const lint = async (config: Config, env: EnvironmentFunctions )

// proposal
export const lint = async (
  config: Pick<Config, "lint" | "languages" | "referenceLanguage">, 
  resources: ast.Resource[], 
)

3. Use the object argument pattern for public-facing APIs

Introducing changes like 1 and 2 becomes non-breaking if the "object argument pattern" is used. The order of the arguments becomes irrelevant. Yes, function overloads could be used but using objects as arguments is superior. No type unions need to be created, old arguments can be soft deprecated, ultimately leading to a better DX and maintainability on our side.

// current
export const lint = async (config: Config, env: EnvironmentFunctions )

// proposal
export const lint = async (args: {
  config: Pick<Config, "lint" | "languages" | "referenceLanguage">, 
  resources: ast.Resource[], 
})

4. Remove redundant query functions

The surface area of the query functions is huge. Mainly driven by separating the type error and warning into dedicated functions. The benefit seems to be non-existent.

• The quantity of function is overwhelming from a DX perspective.
• The quantity of functions leads to maintenance overhead with close to no benefit.
• For example, the getLintReportsByLevel("warning") function serves the same functionality as getLintWarnings().
• Introducing a custom query function for each level has a "big O notation" of n+3.

Current APIs

getLintReports()
getLintReportsByLevel()
getLintErrors() [redundant]
getLintWarnings() [redundant]
getLintReportsWithId() 
getLintErrorsWithId() [redundant]
getLintWarningsWithId() [redundant]
hasLintReports()
hasLintErrors() [redundant]
hasLintWarnings() [redundant]
hasLintReportsWithId()
hasLintErrorsWithId() [redundant]
hasLintWarningsWithId() [redundant]

Proposal a) introduce a type field to all functions

getLintReports(node, ..., level)
getLintReportsByLevel()
getLintReportsWithId() 
hasLintReports()
hasLintReportsWithId()

Proposal b) use the object argument pattern to expose "query arguments" to each function. The API would be severely simplified by only exposing two functions.

PS a good showcase of why the object argument pattern is superior to positional arguments.

getLintReports(node, { level, id, nested } )
hasLintReports(node, { level, id, nested } )

[changeset-bot]

🦋 Changeset detected

Latest commit: 56d1010

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@inlang/core	Minor
@inlang/ide-extension	Minor
@inlang/git-sdk	Minor
@inlang/website	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[render]

Your Render PR Server URL is https://inlang-website-pr-453.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cgbqfpprh17icctm9iog.

[samuelstroschein]

@ivanhofer I refactored the code, please take a look.

• i implemented all suggestions above and went for proposal b with regards to the utility functions
• 100% test coverage remains
• reducing the utility functions lead to a large code footprint reduction
• the standard lint rules should not be impacted by this PR

Learnings

1. I will not rush/LGTM pull requests that expose externally facing APIs.

Exposing 15 utility functions that are more or less one-liner functions increased the source code by over 500 lines. Those 15 functions have been merged into 1 function + 1 utility function, leading to severely fewer lines of code and increasing the DX by requiring to explain less.

Furthermore, I should have seen that you exposed environment functions in the PR but didn't. I have no idea why.

2. I will push harder against extracting one-liner utility functions in the future.

The infamous getLintRulesFromConfig debate made a return when I refactored the code, read #366 (comment) for context. By extracting this one-liner into a dedicated function, you wrote a test that added >30 lines of code. I learned my lesson. I will push hard against extracting one-liner utility functions in the future as they bloat the source code.

The test of the getLintRulesFromConfig utility function:

[ivanhofer]

1. Only use the bare minimum of the config schema

I was lazy on that on. It makes sense to just pick the props that are really required.

2. Remove the environment function from lint

Sure, we can add it if it is needed.

3. Use the object argument pattern for public-facing APIs

Makes sense. I will note that for the future.

4. Remove redundant query functions

This was one of the decisions I always was unsure about. Having a single function looks definitely cleaner 👍

---

Great changes!

Just the thing with the ressources should be considered.

[samuelstroschein]

@ivanhofer#3454 interesting observation. I was adding a return type to lint.

1. Should we return undefined if no lints exists or return the original resource?

// returning undefined makes the consumption harder
const resourcesWithLints = await lint({ config, resources })
if (resourcesWithLints === undefined){
  console.log("no lints")
}
// can't do because lint reports does not expect undefined (rightfully so)
if (hasLintReports(resourcesWithLints) === false){
  //
}

// returning the resources in any case enables the consumer to overwrite the "old resources"  
resources = await lint({ config, resources })
if (hasLintResports(resources){
  // do something
}

2. Why can the lint information be undefined?

type LintInformation = {
	lint?: LintReport[]
}

The DX of lint increases if developers are not forced to do two conditional checks:

1. If lint is undefined
2. If lint is an empty array

current

if (node.lint && node.lint.length > 0){
   // do something
}

proposal

// lint is always defined after running through `lint`, avoiding undefined conditionals
if (node.lint > 0) {
   // do something
}

[ivanhofer]

1. good catch.. we should return the unmodified Resources instead of undefined
2. • That's why we have the query functions.
   • We would need to iterate over all resources at least one time to add that prop. Wasting some compute resources.

[samuelstroschein]

good catch.. we should return the unmodified Resources instead of undefined

I'll implement that!

That's why we have the query functions.
We would need to iterate over all resources at least one time to add that prop. Wasting some compute resources.

Fair. Minor improvement for little upside. I will not implement always defined lints.