Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduces toggle effect & expression #4154
base: dev/feature
Are you sure you want to change the base?
Introduces toggle effect & expression #4154
Changes from 90 commits
ddb40a8
7c6dfb5
f5d6762
9f53cc5
7c799fe
62af771
717cb84
c9c87be
59f8e68
e6ddeea
d6cd4d6
759e1f0
a95f19e
37408c7
dd8c9d0
fb3af6f
2ade451
c4fb185
cb93dd8
63e0ff7
7ac2de5
f6438c8
6f88032
8070373
7f3cef9
8ae20b5
dbc20e6
3075255
8397e24
912f0d8
d3b07a5
0da3b69
4565ff4
026a132
9cc2f23
ef85caf
f6667d7
94dd2b0
d8b81c6
2ffed85
338f097
1fd0cdc
29b1496
a41d1c4
533ecaa
0ca5809
6d33a9c
fa349dc
c51a1ea
7d2b381
63df787
f1c249f
9ca79a9
1611bb3
6cd8135
702206d
876933a
3af8951
c60cd74
0c02931
bb053c0
eab9c59
3bfb357
83e461a
964c998
b3884c0
596644c
ab8b35f
2fb4096
b813df0
e397038
23d711a
832374d
07601b3
08397f5
27b9169
54fbcca
6a3f2a2
0cfa688
e3f1622
68fead4
439639c
9121b2a
64bd9c3
75d999e
c20f01f
14cc4e0
fa7d13c
fc6840c
6cde78c
5d5cb4a
9bb3ed5
ae5960f
8ee2c1b
1315872
bbabd72
b5dd7b5
41a45fe
15a8bf2
7259c54
565fe7d
ab9b399
8bc1de4
adf2438
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We might need to note a behavior change here. For changing something like a list, I believe the indexes will now be lost, which was not prior behaviors. We might choose to only rewrite the list if it contains a boolean?
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.
When you're talking about the list, are you talking about the
toggledExpr
or thefilteredValues
, or even both?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.
if we have a list like
{_values::*}
with:{_values::pickle::name} = "APickledWalrus"
{_values::pickle::likesCake} = true
if we then do
toggle {_values::*}
, the indices such asname
andlikesCake
will be lost (replaced by1
and2
).This is probably unavoidable. However, if we have a list like
{_values::pickle::name} = "APickledWalrus"
{_values::pickle::githubName} = "APickledWalrus"
if we then do
toggle {_values::*}
, there are no booleans to toggle, meaning we should not update the list (and cause it to lose its indices)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.
Looking at the code, don't i already do it?
https://github.com/SkriptLang/Skript/pull/4154/files#diff-4cdf11cdcad1ff81963a3380536868678c5781162395d093ecd42b33c71add01R74
Also, tests are failing, did i misunderstand what you meant?
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.
There may have been some confusion, sorry.
Given a list containing
"one" and "two"
, doingtoggle {_list::*}
should not modify the list since there is nothing to toggle (meaning the indexes will be preserved).However, given a list containing
"one", "two" and true
, doingtoggle {_list::*}
should modify the list since the third value is a boolean (meaning the indexes will be lost).I think the fix you made is correct, but the test is wrong.
Working with the testing you added, you should be testing:
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.
Update: shouldn't we throw an error instead? If the user tries to toggle strings, it shouldn't work. If the variable list includes string, but also boolean, shouldn't we throw an error? Note that i don't think we should throw an error if all values are togglables.
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.
Well, we can't really do runtime errors. I don't think it is just an issue to ignore the values and only change the ones that are actually togglable. You could also opt to not make any changes if there is an invalid type.
The main point still stands, though. If the list contains only blocks, we do not want to actually call the change method as it will reset the indices.
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.
Can you confirm me the commit 8bc1de4 fixes this unexpected behavior?
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.
it looks reasonable
you may want to check out #7120 changeInPlace utility method
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 would need a usage example please. That being said, i will wait for this PR to be merged before doing any change regarding this change.
In terms of organization, either the
changeInPlace
PR is merged before this one, and so i will apply the change here, or this PR is merged before, and so i will apply the change in the other PR.In any case, we need to merge one of them to move on the other.