Skip to content

fix: consider checking params in checkCallback #1324

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
May 27, 2025
Merged

fix: consider checking params in checkCallback #1324

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
687a1c0
chore: fix typings for obsidian 1.9.0
kdnk May 27, 2025
029de94
fix: consider checking parameter in original save callback
kdnk May 27, 2025
5546682
fix: return undefined using void
kdnk May 27, 2025
ca33bf1
fix: use originSaveCallback when checking
kdnk May 27, 2025
ce72500
chore: fix lint
kdnk May 27, 2025
64d00ab
fix: run original checkCallback as well when not checking
kdnk May 27, 2025
25863f0
chore: get back `editor` and `file` definition inside if statement
kdnk May 27, 2025
4f6c735
chore: add missing semicolon
kdnk May 27, 2025
21065d2
chore: remove unnecessary newline
kdnk May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
private overridePaste: boolean = false;
private hasCustomCommands: boolean = false;
private customCommandsLock = new AsyncLock();
private originalSaveCallback?: () => void = null;
private originalSaveCallback?: (checking: boolean) => boolean | void = null;
// The amount of files you can use editor lint on at once is pretty small, so we will use an array
private editorLintFiles: TFile[] = [];
// the amount of files that can be linted as a file can be quite large, so we will want to use a set to make
Expand Down Expand Up @@ -322,15 +322,18 @@
this.originalSaveCallback = saveCommandDefinition?.checkCallback;

if (typeof this.originalSaveCallback === 'function') {
saveCommandDefinition.checkCallback = () => {
this.originalSaveCallback();
saveCommandDefinition.checkCallback = (checking: boolean) => {
this.originalSaveCallback(checking);

if (this.settings.lintOnSave && this.isEnabled) {
const editor = this.getEditor();
if (editor) {
const file = this.app.workspace.getActiveFile();
if (!this.shouldIgnoreFile(file) && this.isMarkdownFile(file) && editor.cm) {
void this.runLinterEditor(editor);
if (!checking) {
this.runLinterEditor(editor);

Check failure on line 334 in src/main.ts

View workflow job for this annotation

GitHub Actions / build (16.x) 

Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator
}
return true;
Copy link
Collaborator

@pjkaufman pjkaufman May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe true should only be returned here if the active file is not null/the editor exists. Checking determines if the command should show.

Copy link
Contributor Author

@kdnk kdnk May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command is overriding editor:save-file, so I'm starting to think that the condition returning true and "lint on save is enabled" are unrelated. In other words, the condition that returns true should be outside the if statement for lintOnSave and isEnabled.

What do you think?

Copy link
Contributor Author

@kdnk kdnk May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should respect the original checkCallback in editor:save-file while checking, so the following code makes sense to me. WDYT?

        if (checking) {
          return this.originalSaveCallback(checking);
        } else {
          const editor = this.getEditor();
          const file = this.app.workspace.getActiveFile();
          if (this.settings.lintOnSave && this.isEnabled) {
            if (editor) {
              if (!this.shouldIgnoreFile(file) && this.isMarkdownFile(file) && editor.cm) {
                void this.runLinterEditor(editor);
              }
            }
          }
        }

Copy link
Collaborator

@pjkaufman pjkaufman May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The checking status should really be handled at the start of the function. If we are checking, we should go ahead and just check right after the logic for running the original callback. If we are checking, we should return whatever the original callback returns. Otherwise we proceed as normal.

Copy link
Collaborator

@pjkaufman pjkaufman May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the change you posted above for the code.

Copy link
Contributor Author

@kdnk kdnk May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjkaufman Thank you!
I committed the changes. Could you review this PR again?

Copy link
Collaborator

@pjkaufman pjkaufman May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. I just did put in another review.

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/typings/obsidian-ex.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ export interface ObsidianCommandInterface {
executeCommandById(id: string): void;
commands: {
'editor:save-file': {
checkCallback(): void;
checkCallback(checking: boolean): boolean | void;
};
};
listCommands(): Command[];
Expand Down
Loading