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

Used string comparisons ignoring case #226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Lyricalpanda
Copy link
Contributor

@Lyricalpanda Lyricalpanda commented Feb 28, 2024

Unsure which of these comparisons needed DisplayName, thought about adding an extension to String for string.caseInsensitive, adding E3.CurrentDisplayName, or a convenience method on E3.

Note: I left the following comparisons as they looked like they wanted case sensitivity

if (spawn.DisplayName == E3.CurrentName)

@@ -65,6 +65,12 @@ public static void PutOriginalTargetBackIfNeeded(Int32 targetid)
}
}
}

public static Boolean CompareInsensitive(this string str, string compareStr)
Copy link
Owner

Choose a reason for hiding this comment

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

change to EqualsIns , as its an equal operator and insensitive is a large word. Otherwise exactly what I meant

Copy link
Contributor Author

@Lyricalpanda Lyricalpanda Feb 28, 2024

Choose a reason for hiding this comment

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

Wee, now I know how to make extensions in C#

Copy link
Owner

@RekkasGit RekkasGit Feb 28, 2024

Choose a reason for hiding this comment

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

or string.Eqic(user)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to call it whatever, even string.RekkaEqic(user) :)

@@ -701,7 +701,7 @@ private static void RegisterEvents()
{
string user = x.args[0];

if (user == E3.CurrentName)
Copy link
Owner

Choose a reason for hiding this comment

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

forgot to do the spawn.DisplayName == E3.CurrentName types

Copy link
Owner

Choose a reason for hiding this comment

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

i just found all occurances of E3.CurrentName

Copy link
Owner

Choose a reason for hiding this comment

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

Also, have to remove the places where the first character is now upper cased as we are allowing anything to be in the config file and making it non case sensitive, so casting target will also have to use the new compairson i think

Copy link
Owner

Choose a reason for hiding this comment

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

thankfully TryByName in the spawns class is case insensitive already

Copy link
Contributor Author

@Lyricalpanda Lyricalpanda Feb 28, 2024

Choose a reason for hiding this comment

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

I left spawn.DisplayName == CurrentName, since I wasn't sure if we wanted to keep those case comparisons and that seemed intentional. It sounds like everything can be case insensitive then? Or are there cases we do care about case?

Copy link
Owner

Choose a reason for hiding this comment

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

other than using the containskey in said if staement

Copy link
Contributor Author

@Lyricalpanda Lyricalpanda Mar 19, 2024

Choose a reason for hiding this comment

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

I was just tracing usage of target for so casting target will also have to use the new compairson i think.

string target = E3.CurrentName
if (!_spawns.TryByName(target, out var spawn)) { }

So target should be fine, as getting/setting should be non case sensitive? At least in buffCheck

And

if (_spawns.TryByName(spell.CastTarget, out var spawn))

Copy link
Owner

Choose a reason for hiding this comment

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

if you read up in the comments, i already said trybyname was fine as it wasn't case sensitive :) its the other stuff tho

Copy link
Owner

Choose a reason for hiding this comment

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

basically anything with a == marks :P

Copy link
Contributor Author

@Lyricalpanda Lyricalpanda Mar 19, 2024

Choose a reason for hiding this comment

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

Oh, well then trybyname is fine :p. I was just trying to be exhaustive around other usages. Only things I saw was the target = E3.CurrentName, Spell.CastTarget = CharacterName, and e3.firstCharToUpper. But my traces seemed like they'd be okay?

Screenshot 2024-03-19 095138
Screenshot 2024-03-19 095149

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