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

Added a function for custom regex replacement #455

Merged
merged 5 commits into from
Nov 19, 2022

Conversation

HardwayLinka
Copy link
Contributor

@HardwayLinka HardwayLinka commented Oct 21, 2022

Hi, I added a new command
image

we can add that command to the Custom Commands
image

Finally, we are able to use custom regular expressions to replace text
regex

I think this is a useful requirement, hope it helps others

@pjkaufman
Copy link
Collaborator

Hey @HardwayLinka . This is a good suggestion. I think it can be really useful.

The only problem I have with it is that it does not really fit under the concept of custom commands in the sense that it is a command you can run without specifying params. Thus it seems like it would be better as its own option of custom rules called custom replacement or something like that. Thus it would not need to be added onto custom commands. I am just not sure how well that would work with the command form of it. I would likely need to refactor it some so that it could both run as a rule and as a command.

What fo you think of my feedback? I would be happy to help implement changes where needed.

@HardwayLinka
Copy link
Contributor Author

HardwayLinka commented Oct 21, 2022

Hi @pjkaufman. Thanks for your patient and detailed feedback.

I agree with what you said. I added this feature to the custom command because it is easier to implement, but at the same time it may be against your design. I will try adding this to a separate option and commit it again when I'm done.

@pjkaufman
Copy link
Collaborator

Hi @pjkaufman. I have a question, I added the feature to the Content Settings, but I can't find the feature under the Content Settings

image

image

Hey @HardwayLinka , I see that you added it as a regular rule. It would need to be a custom rule just like custom commands (just different UI elements).

As for it not showing up, I am guessing you did a search for Content instead of the name of the rule or part of the description, or part of the rule alias. That would be why it did not show up though I might be misunderstanding the screenshot .

Copy link
Collaborator

@pjkaufman pjkaufman left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good, but there are a couple of things I pointed out on the PR. Overall it is looking great!

src/main.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/ui/settings.ts Outdated Show resolved Hide resolved
src/ui/settings.ts Show resolved Hide resolved
src/rules-runner.ts Outdated Show resolved Hide resolved
src/ui/settings.ts Outdated Show resolved Hide resolved
src/ui/settings.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@pjkaufman pjkaufman left a comment

Choose a reason for hiding this comment

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

Minus the one thing about the order in which this runs, it looks great. A benefit to running the custom regexes after the regular linting rules, but before the final linting rules (i.e. here that is to say before line 66) is that it will not cause issues with date modified and other things when running the custom regex.

src/main.ts Outdated Show resolved Hide resolved
src/rules-runner.ts Outdated Show resolved Hide resolved
@pjkaufman
Copy link
Collaborator

Also, sorry about the delay in responding. I was out over the weekend. Thanks for putting in the effort to add this feature!

@HardwayLinka
Copy link
Contributor Author

HardwayLinka commented Nov 2, 2022

Hey @pjkaufman, there's no need to apologize. This project has been very helpful to me, so I'd love to contribute to this project and hope it will help others~

@pjkaufman
Copy link
Collaborator

I will take a look at this hopefully some time this week and then determine if any changes need to be made just from a trail run of it. As far as the code looks, it looks good. I just want to make sure I did not overlook something that would cause an odd user experience.

@pjkaufman
Copy link
Collaborator

This looks good. It looks to cover several different cases as well. I will go ahead and merge this so we can get it out there in the next release. I will also probably add some documentation around this in the README and refactor this and the custom commands into their own components instead of as they are now. Sorry about the delay on getting to this to verify that it looked to work.

Thanks for putting in the work to get this working! I am sure there will be people that find this useful.

@pjkaufman pjkaufman self-requested a review November 19, 2022 15:51
@pjkaufman pjkaufman merged commit da2f4a6 into platers:master Nov 19, 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.

None yet

2 participants