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 regex replace #4323

Open
wants to merge 23 commits into
base: dev/feature
Choose a base branch
from

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Sep 7, 2021

Description

This PR aims to add regex replace to the replace effect

  • Fix one of the replace first patterns not using %strings%in the haystack
  • Use parse tags to clean up syntaxes
  • Overall code cleanup

Note: Currently if there are any regex pattern exceptions/errors they will be ignored.


Target Minecraft Versions: Any
Requirements: None
Related Issues: None

@TPGamesNL TPGamesNL added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Sep 19, 2021
src/main/java/ch/njol/skript/lang/Expression.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

I don't have much to add other than what TP already commented

src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffReplace.java Outdated Show resolved Hide resolved
@TheLimeGlass TheLimeGlass added the 2.8 Targeting a 2.8.X version release label Jan 27, 2023
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

Looks good! Might be worth it to add a test or two.

@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 5, 2023
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 09:07
@AyhamAl-Ali AyhamAl-Ali added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Oct 5, 2023
@sovdeeth sovdeeth added 2.9 Targeting a 2.9.X version release and removed 2.8 Targeting a 2.8.X version release labels Jan 1, 2024
@sovdeeth sovdeeth removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Mar 19, 2024
- Add regex replace first
- removed unused vars
@sovdeeth sovdeeth mentioned this pull request Apr 10, 2024
1 task
@Moderocky Moderocky added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Apr 10, 2024
Comment on lines +56 to +57
"\treplace all \"kys\", \"idiot\" and \"noob\" with \"****\" in the message",
"\treplace using regex \"\\b(kys|idiot|noob)\\b\" with \"****\" in the message # Regex version for better results",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"\treplace all \"kys\", \"idiot\" and \"noob\" with \"****\" in the message",
"\treplace using regex \"\\b(kys|idiot|noob)\\b\" with \"****\" in the message # Regex version for better results",
"\treplace all \"idiot\" and \"noob\" with \"****\" in the message",
"\treplace using regex \"\\b(idiot|noob)\\b\" with \"****\" in the message # Regex version for better results",

maybe it's unnecessary, but i feel like we can keep the docs totally pg while still getting the point across

Comment on lines +68 to +69
"regex:(replace [using] regex|regex replace) %strings% in %strings% with %string%",
"regex:(replace [using] regex|regex replace) %strings% with %string% in %strings%",
Copy link
Member

Choose a reason for hiding this comment

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

we could also just check the matchedPattern here rather than a regex parse tag

@Override
public String toString(@Nullable Event event, boolean debug) {
if (replaceFirst)
return "replace first " + needles.toString(event, debug) + " in " + haystack.toString(event, debug) + " with " + replacement.toString(event, debug)
return "replace first " + needles.toString(event, debug) + (replaceRegex ? " (regex) " : "") + " in " + haystack.toString(event, debug) + " with " + replacement.toString(event, debug)
Copy link
Member

Choose a reason for hiding this comment

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

can be removed as it seems replace first regex is not possible

+ "(case sensitive: " + caseSensitive + ")";
return "replace " + needles.toString(event, debug) + " in " + haystack.toString(event, debug) + " with " + replacement.toString(event, debug)
return "replace " + needles.toString(event, debug) + (replaceRegex ? " (regex) " : "") + " in " + haystack.toString(event, debug) + " with " + replacement.toString(event, debug)
Copy link
Member

Choose a reason for hiding this comment

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

this toString does not seem to reflect the regex replace pattern?

might be better to make a separate case like with replaceFirst. or, it might even be better to just use a stringbuilder.

@APickledWalrus APickledWalrus removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 Targeting a 2.9.X version release bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants