FR: Allow for Warning Mode that Uses Highlighting to Show Areas that Violate Active Rules and Allow User Intervention

[pjkaufman]

Is Your Feature Request Related to a Problem? Please Describe.

When creating and then linting the contents of a file, there may not always be a need to run a rule against an instance of a violation of that rule. It may be that a user wants to pick and choose what scenarios to let the rule violations stay as warnings and what scenarios under which to actually correct a rule violation. This would also allow for the user to see what violations are present in a file prior to making a change.

This is similar to language tool does by adding a class to highlight specific text on the screen.

Describe the Solution You'd Like

I would like to be able to lint a file in warning mode which may be on load in order to see what the rule violations are prior to actually updating the file using the linter. It would also give the choice to the user of when to fix the violation and when to automatically fix it. It would also allow us to add rules that are not automatically fixable.

This is an example of how markdown lint does it in VS Code:

Describe Alternatives You've Considered

Technically you can use other programs from outside of Obsidian to get a list of violations, but it does not account for Obsidian specific logic like frontmatter, tags, etc.

Additional Context

This is also something that would help out with determining what issues seem to exist with certain rules by displaying potential areas to fix for each rule.

@platers, is this something we would consider adding?

[platers]

I think those are all great features to add!

[pjkaufman]

This will require that rules have a method for fixing the rule and one for identifying violations of the rule. It hopefully will not be too hard to implement, but I guess I will not know for sure until I start on it.

[AnnaKornfeldSimpson]

+1 for mode that can list issues in some way instead of auto-fixing!! I use "lint on save" and am very tired of having to iteratively undo all the linter changes when I enable a new rule and discover unintended consequences.

[MiloParigi]

Hi @pjkaufman, bunch of quick notes from looking at the problem for a moment.

Taking a look at the editor.markText method you link to in Language tools, it seems to be coming from the CodeMirror library, which is not currently in this package. In my research, I stumbled upon the Obsidian CodeMirror Option plugin, which is depreciated since Live View came to Obsidian. It might mean that there is a way to achieve this kind of marker directly with the Obsidian Live View API.

Maybe having a separate panel with the current linting error is an easier solution to implement ?

[pjkaufman]

Hey @MiloParigi . You are right that CodeMirror is currently used in the Linter as a dependency. It can be imported as a dev dependency without much effort from my understanding (the plugin template has it).

It is also true that having our own window/panel would be the easiest first step. However that still requires rewriting functions to return the positions in the file and what text to update from each linting rule and adding an ItemView or some kind of panel to the side kind of like the calendar view that is added by Calendar.

Edit: all of that to say, it is possible and easier to not highlight text in the editor view at the start, but it will still take a decent bit of effort to make the change.

[MiloParigi]

I think the best way to address this is to do a really basic POC, with a view somewhere on the screen, and a linter that runs only on the current document.

From a general perspective, what we would have to do would be:

• Modify the current linter framework to allow two different modes, "autofix" and "highlight", with the default being "autofix" in order to not break the current code
• Create a view that will display a list of lint problem (Obsidan Doc - Views)
• Have a hook on content change or a UI button (probably simpler for a first draft) that allows you to run the linter with the runLinterFile method from your main, with the "highlight" option
• Get the result from the linter as a list with the line number, linting error, and linting rule
• Probably a button in the settings to enable/disable this BETA feature

From what you are saying, I understand that the current linter does not return anything ? What code should i investigate to add this functionality ?

Thanks

[pjkaufman]

I think the best way to address this is to do a really basic POC, with a view somewhere on the screen, and a linter that runs only on the current document.

From a general perspective, what we would have to do would be:

• Modify the current linter framework to allow two different modes, "autofix" and "highlight", with the default being "autofix" in order to not break the current code
• Create a view that will display a list of lint problem (Obsidan Doc - Views)
• Have a hook on content change or a UI button (probably simpler for a first draft) that allows you to run the linter with the runLinterFile method from your main, with the "highlight" option
• Get the result from the linter as a list with the line number, linting error, and linting rule
• Probably a button in the settings to enable/disable this BETA feature

This list looks fine except for the return value. The return value should be something like the following:

[
  {
    "oldText": "some text here",
    "newText": "some new text here",
    "position": {
      "start": 128, // some number that is the starting index of the original string
      "end": 160, // some number that is the ending index of the original string
    },
    "rule": "Display Name of the rule that is suggesting the change"
  },
...
]

From what you are saying, I understand that the current linter does not return anything ? What code should i investigate to add this functionality ?

Thanks

The current functionality of the Linter is to take in the contents of the file and run it through all the lint rules until it either hits an error is done if the file is lintable (i.e. a non-ignored markdown file). Each rule takes in a string and an object with all the settings and metadata for that rule in it and returns a string. The returned string is the file contents after the rule ran. Here is where that is defined: https://github.com/platers/obsidian-linter/blob/440fb0fff4bd090a67cbca3c844c3ca771fd3f4d/src/rules.ts#L95-L100

Hopefully that helps with understanding how rules make changes.

[MiloParigi]

Ok, so after looking at the code a bit, here are my thoughts

What the code does currently

There are three main entry points to run the linting process: runLinterEditor, runLinterAllFiles, runLinterFile. runPasteLint is left untouched as there is no reason to change the behavior for this specific purpose. In the end, they each call lintText which is the main function to rework imo.

Possibilities

With the way the rules currently works with the replaced text, there are three ways we could implement it:

• In the Rule object, add a .detect(text) method that will run a regex.find isntead of a regex.replace. It would mean adding this to all existing rules and duplicating each unit tests to test this detect method, which seems like a lot of work.
• In the Rule object, add a .detect(text) method that would basically run the apply and compare the original string with the new one to find which character were modified and where. It is easier to implement at first, but I have no clue on how to compare efficiently the old and new string to get the end position of the modification easily.
• Instead of having it in the Rule object, have the same logic as the second implementation but within the lintText method directly.

For each one, we need to modify the lintText method so I started there

Suggestion

My suggestion is to rework the lintText function and avoid calling the different rules by hand with runBeforeRegularRules, runAfterRegularRules and runCustomRegexReplacement. The idea is to only have one palce in the code where we have the [newText] = Rule.applyIfEnabled(...) call in order to easily maintain the implementation of the highlight. Depending on the mode (highlight or autofix), we would either do the call as shown above, or compare the oldText and newText values, do the comparaison allowing us to get the positions of the replaced characters, and append them to a list of warnings that can then be displayed using the ItemView.

A naive implementation would look like this:

export type RunLinterRulesOptions = {
  [...]
  // Add a linting mode here
  lintingMode: 'autofix' | 'highlight',
}

export class RulesRunner {
  [...]
  lintText(runOptions: RunLinterRulesOptions): string {
    // Variable preparation
    this.skipFile = false;
    const originalText = runOptions.oldText;
    [this.disabledRules, this.skipFile] = getDisabledRules(originalText);

    // Skip the file if it is ignored
    if (this.skipFile) {
      return originalText;
    }

    // Build the rules options and order
    const extraOptions = {
      fileCreatedTime: runOptions.fileInfo.createdAtFormatted,
      fileModifiedTime: runOptions.fileInfo.modifiedAtFormatted,
      fileName: runOptions.fileInfo.name,
      locale: runOptions.momentLocale,
      minimumNumberOfDollarSignsToBeAMathBlock: runOptions.settings.commonStyles.minimumNumberOfDollarSignsToBeAMathBlock,
      aliasArrayStyle: runOptions.settings.commonStyles.aliasArrayStyle,
      tagArrayStyle: runOptions.settings.commonStyles.tagArrayStyle,
      defaultEscapeCharacter: runOptions.settings.commonStyles.escapeCharacter,
      removeUnnecessaryEscapeCharsForMultiLineArrays: runOptions.settings.commonStyles.removeUnnecessaryEscapeCharsForMultiLineArrays,
    };

    const orderedRuleBuilderGroups: Array<RuleBuilderGroups> = [
      {
        logKey: 'logs.pre-rules',
        ruleBuilders: [
          FormatTagsInYaml,
          EscapeYamlSpecialCharacters,
          MoveMathBlockIndicatorsToOwnLine,
        ],
      },
      {
        // Need to add this key that does not exist yet
        logKey: 'logs.regular-rules',
        // Need to get the RuleBuilder instead of the Rule itself
        ruleBuilders: rules.filter((rule) => !rule.hasSpecialExecutionOrder || rule.type !== RuleType.PASTE),
      },
      {
        logKey: 'logs.custom-regex',
        // Need to generate on the fly a RuleBuilder, or just add an exception in the code to keep the current implementation
        ruleBuilders: null,
      },
      {
        logKey: 'logs.post-rules',
        ruleBuilders: [
          CapitalizeHeadings,
          BlockquoteStyle,
          ForceYamlEscape,
          TrailingSpaces,
          YamlTimestamp, // This would need a workaround of some sort to keep the current logic
          YamlKeySort,
        ],
      },
    ];

    const ruleRunningLogText = getTextInLanguage('logs.rule-running');
    const disabledRuleLogText = getTextInLanguage('logs.disabled-text');

    let returnedText = originalText;

    timingBegin(ruleRunningLogText);
    for (const ruleBuilderGroup of orderedRuleBuilderGroups) {
      const ruleBuilderGroupLogText = getTextInLanguage(ruleBuilderGroup.logKey);
      timingBegin(ruleBuilderGroupLogText);

      for (const ruleBuilder of ruleBuilderGroup.ruleBuilders) {
        if (this.disabledRules.includes(ruleBuilder.alias)) {
          logDebug(ruleBuilder.alias + ' ' + disabledRuleLogText);
          continue;
        }
        if (runOptions.lintingMode === 'autofix') {
          [returnedText] = ruleBuilder.applyIfEnabledBase(returnedText, runOptions.settings, extraOptions);
        } else {
          newText = ruleBuilder.applyIfEnabledBase(returnedText, runOptions.settings, extraOptions);
          errorArrayFromSomewhere.push({
            ruleName: ruleBuilder.getName(),
            // Find the position of the replacement(s) here
            startPos: startingPostitonOfReplacement(returnedText, newText),
            endPos: endPositionOfReplacement(returnedText, newText),
          });
        }
      }
      timingEnd(ruleBuilderGroupLogText);
    }
    timingEnd(ruleRunningLogText);

    return returnedText;
  }
  [...]
 }

It's not perfect and I would even say not implementable as is, as for example it would imply in this current form that we are not able to distinct individual replacement from a specific rule (if a rule changed 3-4 occurences of a linting error in a file, I don't know how we would detect that).

Just thought of throwing this out there to start the discussion and see what you would do instead.

Thanks !

[pjkaufman]

Hey @MiloParigi . Thanks for taking time to look into this.

Ok, so after looking at the code a bit, here are my thoughts

What the code does currently

There are three main entry points to run the linting process: runLinterEditor, runLinterAllFiles, runLinterFile. runPasteLint is left untouched as there is no reason to change the behavior for this specific purpose. In the end, they each call lintText which is the main function to rework imo.

I do agree that lintText will likely need rewriting to handle modes in the Linter.

Possibilities

With the way the rules currently works with the replaced text, there are three ways we could implement it:

• In the Rule object, add a .detect(text) method that will run a regex.find isntead of a regex.replace. It would mean adding this to all existing rules and duplicating each unit tests to test this detect method, which seems like a lot of work.
• In the Rule object, add a .detect(text) method that would basically run the apply and compare the original string with the new one to find which character were modified and where. It is easier to implement at first, but I have no clue on how to compare efficiently the old and new string to get the end position of the modification easily.
• Instead of having it in the Rule object, have the same logic as the second implementation but within the lintText method directly.

For each one, we need to modify the lintText method so I started there

I would suggest an overall simpler change which most linters try to use which is returning positional information and what rule has been violated. I would also add the suggested new text. So in implementation that means we would update apply to no longer return text. Instead, it would return a list of starting and ending positions of text it would change and what the resulting value would be after the change. This would allow us to easily replace values in the text and display the suggested output to the user.

This means that there would need to be a method that takes an array of this position and suggestion info and applies it to the provided text. That way you can easily distinguish which changes to make if you need to while still allowing an apply all option. This does require reworking the tests, but that is going to happen with almost any change in this case.

The suggested return value would be

[
  {
    "oldText": "some text here",
    "newText": "some new text here",
    "position": {
      "start": 128, // some number that is the starting index of the original string
      "end": 160, // some number that is the ending index of the original string
    },
    "rule": "Display Name of the rule that is suggesting the change"
  },
...
]

If there is a problem with this route, feel free to mention it. I am not married to this idea, but it seems overall the simplest to me.

Suggestion

My suggestion is to rework the lintText function and avoid calling the different rules by hand with runBeforeRegularRules, runAfterRegularRules and runCustomRegexReplacement. The idea is to only have one palce in the code where we have the [newText] = Rule.applyIfEnabled(...) call in order to easily maintain the implementation of the highlight. Depending on the mode (highlight or autofix), we would either do the call as shown above, or compare the oldText and newText values, do the comparaison allowing us to get the positions of the replaced characters, and append them to a list of warnings that can then be displayed using the ItemView.

It is possible to do this, but I would recommend against just comparing two strings to find the difference to determine what needs to change. This can be done, but the larger the file size, the more taxing this operation becomes. So I would opt to only use it if other ways of handling knowing where a change was made fall through for some reason.

A naive implementation would look like this:

export type RunLinterRulesOptions = {
  [...]
  // Add a linting mode here
  lintingMode: 'autofix' | 'highlight',
}

export class RulesRunner {
  [...]
  lintText(runOptions: RunLinterRulesOptions): string {
    // Variable preparation
    this.skipFile = false;
    const originalText = runOptions.oldText;
    [this.disabledRules, this.skipFile] = getDisabledRules(originalText);

    // Skip the file if it is ignored
    if (this.skipFile) {
      return originalText;
    }

    // Build the rules options and order
    const extraOptions = {
      fileCreatedTime: runOptions.fileInfo.createdAtFormatted,
      fileModifiedTime: runOptions.fileInfo.modifiedAtFormatted,
      fileName: runOptions.fileInfo.name,
      locale: runOptions.momentLocale,
      minimumNumberOfDollarSignsToBeAMathBlock: runOptions.settings.commonStyles.minimumNumberOfDollarSignsToBeAMathBlock,
      aliasArrayStyle: runOptions.settings.commonStyles.aliasArrayStyle,
      tagArrayStyle: runOptions.settings.commonStyles.tagArrayStyle,
      defaultEscapeCharacter: runOptions.settings.commonStyles.escapeCharacter,
      removeUnnecessaryEscapeCharsForMultiLineArrays: runOptions.settings.commonStyles.removeUnnecessaryEscapeCharsForMultiLineArrays,
    };

    const orderedRuleBuilderGroups: Array<RuleBuilderGroups> = [
      {
        logKey: 'logs.pre-rules',
        ruleBuilders: [
          FormatTagsInYaml,
          EscapeYamlSpecialCharacters,
          MoveMathBlockIndicatorsToOwnLine,
        ],
      },
      {
        // Need to add this key that does not exist yet
        logKey: 'logs.regular-rules',
        // Need to get the RuleBuilder instead of the Rule itself
        ruleBuilders: rules.filter((rule) => !rule.hasSpecialExecutionOrder || rule.type !== RuleType.PASTE),
      },
      {
        logKey: 'logs.custom-regex',
        // Need to generate on the fly a RuleBuilder, or just add an exception in the code to keep the current implementation
        ruleBuilders: null,
      },
      {
        logKey: 'logs.post-rules',
        ruleBuilders: [
          CapitalizeHeadings,
          BlockquoteStyle,
          ForceYamlEscape,
          TrailingSpaces,
          YamlTimestamp, // This would need a workaround of some sort to keep the current logic
          YamlKeySort,
        ],
      },
    ];

    const ruleRunningLogText = getTextInLanguage('logs.rule-running');
    const disabledRuleLogText = getTextInLanguage('logs.disabled-text');

    let returnedText = originalText;

    timingBegin(ruleRunningLogText);
    for (const ruleBuilderGroup of orderedRuleBuilderGroups) {
      const ruleBuilderGroupLogText = getTextInLanguage(ruleBuilderGroup.logKey);
      timingBegin(ruleBuilderGroupLogText);

      for (const ruleBuilder of ruleBuilderGroup.ruleBuilders) {
        if (this.disabledRules.includes(ruleBuilder.alias)) {
          logDebug(ruleBuilder.alias + ' ' + disabledRuleLogText);
          continue;
        }
        if (runOptions.lintingMode === 'autofix') {
          [returnedText] = ruleBuilder.applyIfEnabledBase(returnedText, runOptions.settings, extraOptions);
        } else {
          newText = ruleBuilder.applyIfEnabledBase(returnedText, runOptions.settings, extraOptions);
          errorArrayFromSomewhere.push({
            ruleName: ruleBuilder.getName(),
            // Find the position of the replacement(s) here
            startPos: startingPostitonOfReplacement(returnedText, newText),
            endPos: endPositionOfReplacement(returnedText, newText),
          });
        }
      }
      timingEnd(ruleBuilderGroupLogText);
    }
    timingEnd(ruleRunningLogText);

    return returnedText;
  }
  [...]
 }

It's not perfect and I would even say not implementable as is, as for example it would imply in this current form that we are not able to distinct individual replacement from a specific rule (if a rule changed 3-4 occurences of a linting error in a file, I don't know how we would detect that).

Just thought of throwing this out there to start the discussion and see what you would do instead.

Thanks !

The hardest part about this whole thing is that the Linter was designed differently than most linters to where it actually applies changes by default and has no warning mode. This means a lot of positoonal info about what needs changing and where is missing. This is something you have recognized and needs addressing for warning mode to become a reality. That is definitely no easy task as it likely requires rewriting how rules work to get the info we need. As you pointed out, it can be hard to distinguish which changes were made and which one is which after the text has changed. So it is best to do so before the text changes, for performance reasons as well. So far I think this is definitely a start that we can move the discussion forward from. Do you think there is a reason to not go the route of returning position info about what a rule is suggesting be changed and to what?