Skip to content
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

Isn't it possible to enable & disable specific preset by merge-config? #87

Open
r7kamura opened this issue Jan 3, 2024 · 8 comments
Open
Labels
help wanted Extra attention is needed Type: Bug Bug or Bug fixes

Comments

@r7kamura
Copy link

r7kamura commented Jan 3, 2024

As background, I am trying to provide a mechanism to bundle multiple rule presets into one Chrome extension and let users choose which one they want to use.

I tried to implement this using merge-config command while reading the documentation on the following page,

like this:

textlintWorker.postMessage({
  command: "merge-config",
  textlintrc: {
    rules: {
      "preset-ja-technical-writing": true,
      "preset-japanese": false
    },
  },
});

but the above code did not work as intended to toggle disabling/enabling of the entire preset.

Is this currently not feasible without adding individual settings to switch disable/enable for each preset for each rule, as shown below?

textlintWorker.postMessage({
  command: "merge-config",
  textlintrc: {
    rules: {
      "preset-ja-technical-writing": {
        "max-comma": {
          max: Number.MAX_SAFE_INTEGER,
        },
        "sentence-length": {
          max: Number.MAX_SAFE_INTEGER,
        },
        "max-ten": {
          max: Number.MAX_SAFE_INTEGER,
        },
        "max-kanji-continuous-len": {
          max: Number.MAX_SAFE_INTEGER,
          allow: [],
        },
        "arabic-kanji-numbers": false,
        // ...
      },
      "preset-japanese": {
        // ...
      },
    },
  },
});

In this case, though, it makes no sense to use presets in the first place, and it would be easier to use the individual rules without presets.


In addition, I would like to take this opportunity to ask a follow-up question: isn't it possible to "disable a specific rule" in merge-config? I think that just mergeing false for each individual ruleId element won't work. By this issue, the above example also includes a workaround that sets a suitably large value for max.

@azu

This comment was marked as resolved.

@azu azu added the Type: Bug Bug or Bug fixes label Jan 3, 2024
@azu
Copy link
Member

azu commented Jan 3, 2024

Oh, I misread that.

In textlint kernal, presets are expanded as separate rules and then loaded.

in script-compiler

config.filterRules = config.filterRules.map(rule => {
const override = userDefinedConfig.filterRules.find(o => o.ruleId === rule.ruleId);
return { ...rule, ...override };
});
}
if (userDefinedConfig.plugins) {
config.plugins = config.plugins.map(rule => {
const override = userDefinedConfig.plugins.find(o => o.pluginId === rule.pluginId);
return { ...rule, ...override };
});
}
};
self.addEventListener('error', (event) => {
self.postMessage({
command: "error",
// wrapping any type error with Error
error: new Error("unexpected error", {
cause: event.error
})
})
});

in textlint
https://github.com/textlint/textlint/blob/e184d1f33340bb3fc0bc13c50f62d8feb6ecae8a/packages/%40textlint/config-loader/src/loader.ts#L215-L223

Probably, We need to treat preset-abc: false as preset-abc/a: false, preset-abc/b: false .... in merge-config command as special case.

config.filterRules = config.filterRules.map(rule => {
const override = userDefinedConfig.filterRules.find(o => o.ruleId === rule.ruleId);
return { ...rule, ...override };
});
}
if (userDefinedConfig.plugins) {
config.plugins = config.plugins.map(rule => {
const override = userDefinedConfig.plugins.find(o => o.pluginId === rule.pluginId);
return { ...rule, ...override };
});
}
};
self.addEventListener('error', (event) => {
self.postMessage({
command: "error",
// wrapping any type error with Error
error: new Error("unexpected error", {
cause: event.error
})
})
});

@azu
Copy link
Member

azu commented Jan 3, 2024

In addition, I would like to take this opportunity to ask a follow-up question: isn't it possible to "disable a specific rule" in merge-config?

It is possible that the process of excluding rules with an option of false from the config may be missing.
The "merge-config" has not been tested and should be re-created.

@azu

This comment was marked as outdated.

@azu azu added the help wanted Extra attention is needed label Jan 3, 2024
@r7kamura
Copy link
Author

r7kamura commented Jan 6, 2024

I see. I like the idea of expanding { presetA: false } that way.

Reviewing from the design of config, I see there are two options on how to handle { ruleA: false } here.

One is to leave the current config as it is in its current design and regard false as a special instruction to delete the element when merge is performed, as you described.

The other is to design the config so that false is a special setting that disables the rule, so that when you merge, you do nothing special and false is the setting value of the rule. This way, I think it will work well not only when merge, but also when writing { ruleA: false } in .textlintrc, so there are fewer surprises and more symmetry.

@azu
Copy link
Member

azu commented Jan 7, 2024

#87 (comment)
This comment is probably wrong and hide it.

  • In textlint:
    • @textlint/config-loader expands preset to rules before passing config to kernel
    • ℹ️ This limitation come from that kernel can not use Node.js API like node:fs
  • In editor script:
    • Already expanded rules exists
    • editor script can not list rules from preset name unless pass more info

@azu
Copy link
Member

azu commented Jan 8, 2024

The other is to design the config so that false is a special setting that disables the rule, so that when you merge, you do nothing special and false is the setting value of the rule. This way, I think it will work well not only when merge, but also when writing { ruleA: false } in .textlintrc, so there are fewer surprises and more symmetry.

Can you explain the detail?
I couldn't imagine it imho.

@r7kamura
Copy link
Author

r7kamura commented Jan 8, 2024

The other is to design the config so that false is a special setting that disables the rule, so that when you merge, you do nothing special and false is the setting value of the rule. This way, I think it will work well not only when merge, but also when writing { ruleA: false } in .textlintrc, so there are fewer surprises and more symmetry.

Can you explain the detail? I couldn't imagine it imho.

const config1 = {
  rules: {
    a: { /* ... */ }
  }
};

const config2 = {
  rules: {
    a: false
  }
};

const result = mergeConfig(config1, config2)

In the code above, I'm talking about whether the result should be { rules: {} } or { rules: { a: false } }.
In the latter case, the responsibility for handling the false would be of the config value handler, not the merge handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed Type: Bug Bug or Bug fixes
Projects
None yet
Development

No branches or pull requests

2 participants