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

[Bracket Checker] Also check for the order of brackets #16718

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

Haoming02
Copy link
Contributor

@Haoming02 Haoming02 commented Dec 12, 2024

Simple Description

  • Currently, this script only checks if the number of opening and closing brackets are the same. So if you have mistyped something like )...( in the prompts, the checker would not detect it as error.

  • The current regular expression is frankly... difficult to read

    • Also, regexr gave a warning that "negative lookbehind" ((?<!) may not be supported on all browsers; though when I looked it up, it seems to be supported on all modern browsers.

List of Changes

  • Manually loop through the text to check for the order of brackets
  • Now also checks for pairing of escaped brackets
  • Use the built-in onEdit function to detect user inputs
  • Converted to use the IIFE pattern
    • To revert this, also move the const pairs = [ ... ] inside checkBrackets() { ... }

  • Edited the credits - The only mention of "Hingashi no Florin" I can ever find online is this line, which is probably the original author of this script, Bwin4L's nickname? Do we keep it?

Checklist


Performance: Checking a textbox filled to ~150 tokens took 0.2 ms

Copy link
Collaborator

@w-e-w w-e-w left a comment

Choose a reason for hiding this comment

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

  • Now also checks for pairing of escaped brackets (ie. ( ... ), etc)

literal brackets should not be checked as it is not a part of the prompt syntax
it is valid to have mismatch literal brackets

@Haoming02
Copy link
Contributor Author

literal brackets should not be checked as it is not a part of the prompt syntax

Maybe make it a setting then? since:

it is valid to have missed matched literal brackets

Who would have a non-closed brackets anyway? I can't think of a reason to only use either opening or closing bracket alone.

Take Booru Tags for example, literal brackets are used to denote franchise or medium, and they are always in pairs.

@w-e-w
Copy link
Collaborator

w-e-w commented Dec 13, 2024

to be honest I don't know if anyone would ever find that useful but out of curiosity I did a strange test
and I'm not sure what to think about the results
20241213-180417-043390 2955369934 8683
no escape, all literal, only matching, on parentheses, blank

prompt

((((( ( ⊃・ω・)⊃☂

xyz s/r

((((( ( ⊃・ω・)⊃☂, \(\(\(\(\( \( ⊃・ω・\)⊃☂, \( ⊃・ω・\)⊃☂, ⊃・ω・⊃☂,

@w-e-w
Copy link
Collaborator

w-e-w commented Dec 13, 2024

this result is kind of interesting
20241213-181759-915867 530718384 c44e
20241213-181812-598092 151051099 f7ab
20241213-181824-698849 2589342750 e5f1
20241213-181857-673344 4110295215 7c84

\(\(\(girl
\(\(\(girl,\(\(girl,\(girl,girl,girl\),girl\)\),girl\)\)\)

@Haoming02
Copy link
Contributor Author

I don't know if anyone would ever find that useful

If you meant checking the pairings, it's for when you forgot to escape both brackets (eg. amiya \(arknights) would now show as error)

@w-e-w
Copy link
Collaborator

w-e-w commented Dec 13, 2024

I don't know if anyone would ever find that useful

no actually is the reverse
I agree with you most people would have matched literal brackets
I'm not sure how useful Not matching mismatched literal brackets is useful

maybe I'm thinking too much
my issue is more on what the "error" message is conveying

I feel like using the same "error message" is misleading
for one it is never an error missmatch brackets are handled internally using the fallback behaviorit, likely not what the user intended, but it is not an error

when ther is a mismatch bracket a "RED error" appear let the user know something worng and would trigger the fallback behavior
but when mismatched literal bracket trigger use the same "RED error" it implies that something is wrong and would trigger fallback behavior, but this is not the case,

I won't have an issue if it instead is highlighted like yellow when it is caused by mismatch literal brackets
more like a warning as opposed to a error

@Haoming02 Haoming02 marked this pull request as draft December 13, 2024 10:10
@Haoming02
Copy link
Contributor Author

Now that I think about it, in case of amiya \(arknights), the current script would've shown as error anyway, as there is now a lone closing bracket.

And in case of amiya \(arknights, it frankly does not matter too much. And as you mentioned, it does not actually cause any error.


  • TL;DR: Reverted checking the pairings of escaped brackets

@Haoming02 Haoming02 marked this pull request as ready for review December 16, 2024 01:56
counts[bracket[1]] = (counts[bracket[1]] || 0) + 1;
});
const errors = [];
(function() {
Copy link
Collaborator

@w-e-w w-e-w Dec 17, 2024

Choose a reason for hiding this comment

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

functionally wise looks good to me
just one more question / issue

is there a particular reason that you decided to wrap this entire thing inside a function?

(function() {
    /*...*/
})();

if you did this because to avoid namespace collisions
considering that all these functions already existed for a long time in global space
there shouldn't be any collision issues
apart from only new variable added pairs
I would suggest you rename pairs to something more like promptBracketCheckerPairs
and leave the structure other as is

or is there some other considerations that makes you move this entire thing into a function?
I feel like it's advantageous to allow other scripts to have access to pairs / promptBracketChecker
this way for some reason an extension wishes to add some additional syntax they could

@Haoming02
Copy link
Contributor Author

Yeah, it's mainly to avoid namespace pollution afaik

I first learnt about this pattern (called IIFE) from the Forge ControlNet

@w-e-w
Copy link
Collaborator

w-e-w commented Dec 17, 2024

LGTM


oh there's a name for this pattern
I know what this does but I never knew that it was a name for it


another issue with using IIFE in this case while this is a built-in "extention" (so can be disabled)
but beeing "built-in" they could be an extension adds a custom input of somekind that also uses checkBrackets()
so if checkBrackets() is is move into a IIFE, things may not work even when thay should

@w-e-w w-e-w merged commit e8c3b1f into AUTOMATIC1111:dev Dec 17, 2024
3 checks passed
@Haoming02 Haoming02 deleted the bracket-checker-order branch December 18, 2024 01:41
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.

2 participants