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

FR: Trigger custom events on linting #1215

Open
baodrate opened this issue Nov 6, 2024 · 4 comments
Open

FR: Trigger custom events on linting #1215

baodrate opened this issue Nov 6, 2024 · 4 comments
Labels
rule suggestion Suggestion to add or edit a rule

Comments

@baodrate
Copy link

baodrate commented Nov 6, 2024

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

Currently, the only way to add custom behavior when files are linted is to set a custom command. obsidian-linter will call this command after a file is linted. The consequences of this approach:

  • files are not passed directly to the custom command. custom commands are expected to operate on workspace.getActiveFile().
    • the current approach is to open the file in a sidebar tab before calling the command. the downside of this is that it is quite slow and fragile
    • quoting the current code: "Linting multiple files with custom commands enabled is a slow process that requires the ability to open panes in the side panel. It is noticeably slower than running without custom commands enabled"
  • custom commands are always run after linting is complete
    • they do not affect the diff reporting, and multiple linter runs might be required to properly format the file

Describe the Solution You'd Like

Trigger custom events prior to and after files are linted, allowing users to define custom lint functions simply by subscribing to the events

Examples of plugins that expose events for other plugins to subscribe to:

Please include an example where applicable:

Example implementation

Adding event triggers to obsidian-linter
diff --git a/src/main.ts b/src/main.ts
index 923661e..a161bd3 100644
--- a/src/main.ts
+++ b/src/main.ts
@@ -433,11 +433,19 @@ export default class LinterPlugin extends Plugin {
   }
 
   async runLinterFile(file: TFile, lintingLastActiveFile: boolean = false) {
+    this.app.workspace.trigger('linter:pre-lint-file', file);
     const oldText = stripCr(await this.app.vault.read(file));
+    const lintInfo = {
+      file: file,
+      contents: oldText,
+    };
+    this.app.workspace.trigger('linter:lint-begin', lintInfo);
     const newText = this.rulesRunner.lintText(createRunLinterRulesOptions(oldText, file, this.momentLocale, this.settings, this.defaultAutoCorrectMisspellings));
+    this.app.workspace.trigger('linter:lint-end', lintInfo);
 
     if (oldText != newText) {
       await this.app.vault.modify(file, newText);
+      this.app.workspace.trigger('linter:post-lint-file', file, oldText);
 
       if (lintingLastActiveFile) {
         const message = getTextInLanguage('logs.file-change-lint-message-start') + ' ' + this.lastActiveFile.path;
@@ -454,6 +462,7 @@ export default class LinterPlugin extends Plugin {
 
       return;
     }
+    this.app.workspace.trigger('linter:post-lint-file', file, oldText);
 
     await this.runCustomCommandsInSidebar(file);
   }
@@ -523,21 +532,27 @@ export default class LinterPlugin extends Plugin {
 
     logInfo(getTextInLanguage('logs.linter-run'));
 
+    this.app.workspace.trigger('linter:pre-lint-editor', editor);
     const file = this.app.workspace.getActiveFile();
     const oldText = editor.getValue();
-    let newText: string;
+    const lintInfo = {
+      file: file,
+      contents: oldText,
+    };
+    this.app.workspace.trigger('linter:lint-begin', lintInfo);
     try {
-      newText = this.rulesRunner.lintText(createRunLinterRulesOptions(oldText, file, this.momentLocale, this.settings, this.defaultAutoCorrectMisspellings));
+      lintInfo.contents = this.rulesRunner.lintText(createRunLinterRulesOptions(oldText, file, this.momentLocale, this.settings, this.defaultAutoCorrectMisspellings));
     } catch (error) {
       this.handleLintError(file, error, getTextInLanguage('commands.lint-file.error-message') + ' \'{FILE_PATH}\'', false);
       return;
     }
-
-    const changes = this.updateEditor(oldText, newText, editor);
+    this.app.workspace.trigger('linter:lint-end', lintInfo);
+    const changes = this.updateEditor(oldText, lintInfo.contents, editor);
     const charsAdded = changes.map((change) => change[0] == DiffMatchPatch.DIFF_INSERT ? change[1].length : 0).reduce((a, b) => a + b, 0);
     const charsRemoved = changes.map((change) => change[0] == DiffMatchPatch.DIFF_DELETE ? change[1].length : 0).reduce((a, b) => a + b, 0);
 
     this.displayChangedMessage(charsAdded, charsRemoved);
+    this.app.workspace.trigger('linter:post-lint-editor', editor, oldText);
 
     // run custom commands now since no change was made
     if (!charsAdded && !charsRemoved) {
@@ -546,7 +561,7 @@ export default class LinterPlugin extends Plugin {
       this.editorLintFiles.push(file);
     }
 
-    this.updateFileDebouncerText(file, newText);
+    this.updateFileDebouncerText(file, lintInfo.contents);
 
     setCollectLogs(false);
   }
custom lint function using event subscribers

This uses obsidian-custom-js.

class ExtraLintEvent {
	constructor() {
		this.app = window.customJS.app;
		this.preLintFileHandler = this.preLintFileHandler.bind(this);
		this.postLintFileHandler = this.postLintFileHandler.bind(this);
		this.lintBeginHandler = this.lintBeginHandler.bind(this);
	}

	async invoke() {
		this.app.workspace.on('linter:pre-lint-file', this.preLintFileHandler);
		this.app.workspace.on('linter:post-lint-file', this.postLintFileHandler);
		this.app.workspace.on('linter:lint-begin', this.lintBeginHandler);
	}

	deconstructor() {
		this.app.workspace.off('linter:pre-lint-file', this.preLintFileHandler);
		this.app.workspace.off('linter:post-lint-file', this.postLintFileHandler);
		this.app.workspace.off('linter:lint-begin', this.lintBeginHandler);
	}

	preLintFileHandler(file) {
		const app = window.customJS.app;
		app.fileManager.processFrontMatter(file, (frontmatter) => {
			frontmatter['foo'] = "baz";
		});
	}

	postLintFileHandler(file, oldText) {
		const app = window.customJS.app;
		const newText = this.app.vault.read(file)
		if (oldText !== newText) {
			app.fileManager.processFrontMatter(file, (frontmatter) => {
				frontmatter['linted'] = true;
			});
		}
	}

	lintBeginHandler(info) {
		info.contents = info.contents.replace(/foo/g, 'bar');
	}
}

Describe Alternatives You've Considered

  • Use the custom commands setting
    • downsides described in first section
    • cannot run prior to linting
  • Using default file create/modify events
    • not specific enough (to only linting actions)
    • also cannot run prior to linting

Additional Context

The example implementation I provided adds these events:

  • linter:post-lint-file
  • linter:post-lint-editor
  • linter:lint-begin
  • linter:lint-end
  • linter:pre-lint-file
  • linter:pre-lint-editor

I believe they cover most of the possible use cases I can envision.

  • linter:pre-lint-file / linter:pre-lint-editor
    • callback signatures: (file: TFile) => void / (editor: Editor) => void
    • prior to this.app.vault.read(file) / editor.getValue()
  • linter:lint-begin
    • callback signature: (info: { file: TFile, contents: string }) => void
    • after reading file/editor, prior to builtin lints
  • linter:lint-end
    • callback signature: (info: { file: TFile, contents: string }) => void
    • after builtin lints, prior to writing
  • linter:post-lint-file / linter:post-lint-editor
    • callback signatures: (file: TFile, oldText: string) => void / (editor: Editor, oldText: string) => void
    • after linting and writing

The lint-begin and lint-end events act as hooks to allow users to implement custom linting steps, which run either before or after all of the builtin steps

The (pre|post)-lint-(file|editor) events are intended to allow users to perform some operations outside of the chain of linting operations (all linting should be pending/completed at these points)

@baodrate baodrate added the rule suggestion Suggestion to add or edit a rule label Nov 6, 2024
@pjkaufman
Copy link
Collaborator

pjkaufman commented Nov 6, 2024

Hey @baodrate . Thank you for the feature request. I think this is a good start to a discussion. I am not 100% sure if the Linter is ready for this kind of functionality yet since most of the linting rules are planned to be moved to a web worker. Would doing so cause problems with this proposed eventing system?

If so, do you have any ideas about mitigating this?

@baodrate
Copy link
Author

baodrate commented Nov 6, 2024

Would doing so cause problems with this proposed eventing system?

This is perhaps a premature answer (I've only skimmed #1047), but I'm inclined to say that it shouldn't. Or more precisely, the change of offloading the data-intensive work onto webworker threads should not make implementing the event triggers any more difficult. However, since the workers cannot directly interact with obsidian's event system, the events would still be run in the main thread (which shouldn't be too much of an issue, IMO, since it'd be a purely opt-in advanced feature)

As for the implementation, the only thing that should change is where the events are triggered.

So for https://github.com/pjkaufman/obsidian-linter/blob/inline-worker/src/rules-runner/file-lint-manager.ts

something like this, perhaps:

--- foo1.js	2024-11-06 05:16:17
+++ foo2.js	2024-11-06 05:26:35
@@ -42,6 +42,7 @@
 			const worker = Worker();
 			worker.onmessage = async (resp: any) => {
 				await this.finish(resp.data as RunLinterRulesOptions, index);
+				this.app.workspace.trigger('linter:post-lint-file', resp.data);
 			};
 
 			this.workers.push(worker);
@@ -82,6 +83,7 @@
 
 		// Queue a new job onto this worker.
 		const job = this.lintQueue.shift();
+		this.app.workspace.trigger('linter:pre-lint-file', data);
 		if (job !== undefined) {
 			this.send(job, index);
 		}
@@ -114,6 +116,7 @@
 			dateModifiedKey: yamlTimestampOptions.dateModifiedKey,
 		});
 
+		this.app.workspace.trigger('linter:lint-end', data);
 		if (this.callbacks.has(data.fileInfo.path)) {
 			const callback = this.callbacks.get(data.fileInfo.path);
 			this.callbacks.delete(data.fileInfo.path);
@@ -131,6 +134,7 @@
 		this.busy[workerId] = true;
 		this.vault.read(file).then((oldText: string) => {
 			const lintRunnerSettings = createRunLinterRulesOptions(stripCr(oldText), file, this.momentLocale, this.settings);
+			this.app.workspace.trigger('linter:lint-begin', lintRunnerSettings);
 			this.workers[workerId].postMessage(lintRunnerSettings);
 		});
 	}

please correct me if i've misunderstood something

@pjkaufman
Copy link
Collaborator

That sounds right, but I have not dealt with event triggers and whatnot before. So I will need to see about testing this whenever work happens on it. I would prefer to fix some more bugs then get the web worker logic present before taking a stab at this, but I am so scatterbrained at times I may forget about this ticket.

I want to make it clear, that I am not opposed to this addition. I am just not yet sure of some of the intricacies or edge cases that may exist.

@baodrate
Copy link
Author

baodrate commented Nov 6, 2024

I totally get it. I'll wait until the web worker stuff is more finalized and then I'll revisit this ticket. cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule suggestion Suggestion to add or edit a rule
Projects
None yet
Development

No branches or pull requests

2 participants