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

Add Folder Lint Option to Right Click Folder Action Menu #199

Merged
merged 5 commits into from
May 13, 2022

Conversation

pjkaufman
Copy link
Collaborator

@platers

Fixes #177

I essentially duplicated the confirmation model and changed a few things as well as making sure that the event action would be added to the drop down menu when your right click a folder.

I have a couple of questions regarding how things were done here and whether I should go ahead and try to change a few things:

  • Is there a more performant way to determine which files are in the folder selected or do I just have to iterate over all files and just see if they start with the same path as the folder selected?
  • Can the modal code be made generic to allow for the addition of a couple of extra params to cut down on the duplication of code? I was finding it hard to pass in a function as a param.
  • Do we want to try to account for the wording that would be needed for folders with only 1 markdown file in it?
  • Do we want the confirmation modal for the request to lint the files in the folder?
  • Is it logical to assume that linting the folder will lint all subfolders as well?
  • Are there any documentation updates or UT updates that are needed?

Thanks for the guidance on this. I am hoping this will benefit those that use the plugin and hopefully be straight forward in the code.

…t folder (the underlying code duplicates the confirmation modal code and slightly modifies the linting rule for all files for just the folder)
@platers
Copy link
Owner

platers commented May 12, 2022

Thanks for the PR! I'm checking it out now. I'm having trouble finding the button is to lint folders, can you send a screenshot? Also it would be nice to have a command to lint files in the current folder.

@pjkaufman
Copy link
Collaborator Author

Thanks for the PR! I'm checking it out now. I'm having trouble finding the button is to lint folders, can you send a screenshot? Also it would be nice to have a command to lint files in the current folder.

You need to right click on the folder to get it to to show up in the list. As far as adding the command goes, that should be relatively easy since it will just require a refactor to get the linting code reusable from both places.

Here is a screenshot:
Screenshot from 2022-05-12 17-34-59

@pjkaufman
Copy link
Collaborator Author

The command is now present.

@pjkaufman
Copy link
Collaborator Author

Please let me know if the command seems to act weird.

src/main.ts Outdated
console.log('Linting folder ' + folder.name);
let lintedFiles = 0;
await Promise.all(this.app.vault.getMarkdownFiles().map(async (file) => {
if (file.path.startsWith(folder.path) && !this.shouldIgnoreFile(file)) {
Copy link
Owner

Choose a reason for hiding this comment

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

when I lint the folder named Test it also lints files starting with Test like Testing.md which it shouldnt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be taken care of with the latest commit.

@platers
Copy link
Owner

platers commented May 12, 2022

@platers

Fixes #177

I essentially duplicated the confirmation model and changed a few things as well as making sure that the event action would be added to the drop down menu when your right click a folder.

I have a couple of questions regarding how things were done here and whether I should go ahead and try to change a few things:

  • Is there a more performant way to determine which files are in the folder selected or do I just have to iterate over all files and just see if they start with the same path as the folder selected?

I dont know a better way

  • Can the modal code be made generic to allow for the addition of a couple of extra params to cut down on the duplication of code? I was finding it hard to pass in a function as a param.

Yeah it would be nice to reuse code there. Maybe a more generic Modal class? I'm ok with leaving it as is though.

  • Do we want to try to account for the wording that would be needed for folders with only 1 markdown file in it?

Why are folders with 1 file special?

  • Do we want the confirmation modal for the request to lint the files in the folder?

I think its a good idea since users wont be able to see the files being linted when they run it.

  • Is it logical to assume that linting the folder will lint all subfolders as well?

Makes sense.

  • Are there any documentation updates or UT updates that are needed?

Could you document this feature to the README? Just a sentence or two in the usage section. Make sure to add it in the readme_template and not the README.md since its generated.

Thanks for the guidance on this. I am hoping this will benefit those that use the plugin and hopefully be straight forward in the code.

Thanks for the contribution, it looks basically good to me! Just noticed one bug that should be a quick fix

pjkaufman added 2 commits May 12, 2022 23:34
…ntation image showing the ability to get the lint folder option in the right click menu
@pjkaufman
Copy link
Collaborator Author

@platers
Fixes #177
I essentially duplicated the confirmation model and changed a few things as well as making sure that the event action would be added to the drop down menu when your right click a folder.
I have a couple of questions regarding how things were done here and whether I should go ahead and try to change a few things:

  • Is there a more performant way to determine which files are in the folder selected or do I just have to iterate over all files and just see if they start with the same path as the folder selected?

I dont know a better way

  • Can the modal code be made generic to allow for the addition of a couple of extra params to cut down on the duplication of code? I was finding it hard to pass in a function as a param.

Yeah it would be nice to reuse code there. Maybe a more generic Modal class? I'm ok with leaving it as is though.

  • Do we want to try to account for the wording that would be needed for folders with only 1 markdown file in it?

Why are folders with 1 file special?

  • Do we want the confirmation modal for the request to lint the files in the folder?

I think its a good idea since users wont be able to see the files being linted when they run it.

  • Is it logical to assume that linting the folder will lint all subfolders as well?

Makes sense.

  • Are there any documentation updates or UT updates that are needed?

Could you document this feature to the README? Just a sentence or two in the usage section. Make sure to add it in the readme_template and not the README.md since its generated.

Thanks for the guidance on this. I am hoping this will benefit those that use the plugin and hopefully be straight forward in the code.

Thanks for the contribution, it looks basically good to me! Just noticed one bug that should be a quick fix

The special case I was referring to in with 1 file is that when it gets done linting it will say "All 1 files in FOLDER_NAME". I am not sure if there is a different set of wording we would like.

However, I was able to make the modal generic and be used by both.

I updated the README template to include the info and to update the contribution steps to include the mentioning of forking the repo since one cannot update this repo directly by default.

@pjkaufman pjkaufman requested a review from platers May 13, 2022 04:01
Copy link
Owner

@platers platers left a comment

Choose a reason for hiding this comment

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

LGTM

@platers platers merged commit 270563c into platers:master May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Lint files in folder
2 participants