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

Reformat renatoliveira's patch #30

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

Conversation

vr8hub
Copy link

@vr8hub vr8hub commented Jun 2, 2020

This started out as a reformatted PR#13 per the request from @kevinohara80 there. (I asked there if renatoliveira was OK with me submitting a reformatted version, and he said he was). I added the following changes:

  1. Restored the clearAllBypasses method, and made it work for either manually bypassed handlers or a globalBypass.
  2. Removed the new clearGlobalBypass method since 1 above handles it as well.
  3. Changed the isBypassed methods to return true if a globalBypass was in effect.
  4. Restructured the ifs in setTriggerContext to make the block a bit more efficient.
  5. Updated the README to include examples of bypassing a list and using globalBypass().

Comment on lines +131 to +133
public static void showLimits() {
showLimits = true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Overloaded to allow for calling method with a boolean. This allows toggling off.

Suggested change
public static void showLimits() {
showLimits = true;
}
public static void showLimits() {
showLimits(true);
}
public static void showLimits(Boolean enabled) {
showLimits = enabled;
}

@vr8hub
Copy link
Author

vr8hub commented Jun 13, 2020

Yes, I thought it was odd it couldn't be turned off, but didn't think enough to change it. :) Fixed. I also fixed the global bypass example in the README.
(I haven't encountered a "suggestion" before with a code change; if I could have committed it directly I apologize, I just went with what I know works. And I needed to fix the readme anyway.)

@kevinohara80
Copy link
Owner

@vr8hub No problem. Thanks for the fix. Looks like there is a merge conflict that needs to be resolved before merging. If you can take care of that I can get this merged.

@vr8hub
Copy link
Author

vr8hub commented Jul 15, 2020

Well, I'm out of my depth, apparently. I've never had something change underneath me on a PR.
I modified the branch to include the use of switch and fix another issue, but it still reports the same conflicts. And the two conflicts it's reporting are both changes I'm making. So I don't see how those could be conflicts. Sorry, I don't use git in a distributed environment, so I've never had to solve these types of problems.

@vr8hub
Copy link
Author

vr8hub commented Feb 10, 2021

I finally had time to investigate this and believe the conflicts are resolved.

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