-
Notifications
You must be signed in to change notification settings - Fork 112
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.
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
Conversation
pjkaufman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. There are just a couple of recommended changes.
src/main.ts
Outdated
| if (!checking) { | ||
| this.runLinterEditor(editor); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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);
}
}
}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pjkaufman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are two lint issues plus a change in the actual meaning of the save callback logic added by the Linter.
src/main.ts
Outdated
| 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); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here should be the same as the original to avoid trying to act on null or undefined files:
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) {
if (!checking) {
this.runLinterEditor(editor);
}
return true;
}
}
}
};
pjkaufman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for making this change.
Fix #1315 (comment)
ref. #1314