-
Notifications
You must be signed in to change notification settings - Fork 379
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 swap command to autoattach module #1582
base: master
Are you sure you want to change the base?
Conversation
This allows you do swap the position of two rules in execution order To make this work well, we needed to change how settings for the module are stored. This includes a function and check to migrate existing rules over.
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.
Please add a test.
@@ -116,22 +118,36 @@ class CChanAttach : public CModule { | |||
} | |||
} | |||
|
|||
void HandleSwap(const CString& sLine) { | |||
u_int iA = sLine.Token(1).ToUInt(); |
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.
Where does this typedef come from?
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.
At least on my machine it is part of the system headers. I was looking at how one of the other modules handled numeric input and copied it from there.
modules/autoattach.cpp
Outdated
void HandleList(const CString& sLine) { | ||
int Index; |
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.
iIndex
? please follow the style around...
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.
Will push a fix for this and other some of the other issues soon.
modules/autoattach.cpp
Outdated
} | ||
|
||
delete Index; |
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.
WAT?
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.
Sorry, my C++ is really rusty. Will push with fix soon.
modules/autoattach.cpp
Outdated
|
||
Add(bNegated, sChan, sSearch, sHost); | ||
} | ||
Save(); |
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.
Why?
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.
Because I apparently can't read and misunderstood was some of what the original code was doing. Push coming soon.
modules/autoattach.cpp
Outdated
} | ||
|
||
|
||
for (VCString::const_iterator rit = rules.begin(); rit != rules.end(); |
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.
Why not foreach loop?
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.
For what you're trying to do, inserting a new rule into a specified position would probably more useful than swapping 2 rules
test/integration/tests/modules.cpp
Outdated
client.Write("PRIVMSG *autoattach :Add * test *"); | ||
client.ReadUntil("Added to list"); | ||
client.Write("PRIVMSG *autoattach :Swap 1 2") | ||
client.ReadUntil("Rules Swapped"); |
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 can show you much easier implementation which will still pass this test:
AddCommand("Swap", "", "",
[=](const CString& sLine) { PutModule("Rules Swapped"); });
Can you code here the exact issue which you were facing without this new functionality in place? That's the point of having a test.
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.
Okay, I am working on expanding this, but the thing I'm not sure how to test is the client correctly not attaching when the ignore rule is in place.
modules/autoattach.cpp
Outdated
return false; | ||
} else { | ||
std::iter_swap(m_vMatches.begin() + (Start - 1), | ||
m_vMatches.begin() + (End - 1)); |
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.
std::swap(matches[start], maches[end]);
?
modules/autoattach.cpp
Outdated
return true; | ||
} | ||
|
||
bool Swap(int Start, int End) { |
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.
Something is wrong with naming of the variables. Where is the prefix?
…/znc into autoattach-improvements
This allows you do swap the position of two rules in the order
To make this work well, we needed to change how settings for the mofule
are stored. This includes a function and check to migrate existing rules over.