Closed DylanHolmesDH closed 9 years ago
rourke750
commented
9 years ago @ttk2 Any feedback on this? This pull request is fine as it fixes readability. @KillerSmurf This should really go in the morning change log. Also you should make an issue instead of a pull request in the future.
ttk2
commented
9 years ago this should go in to the changelog, so why merge it early roruke?
On Wed, Oct 8, 2014 at 3:38 PM, rourke750 notifications@github.com wrote:
Merged #3 https://github.com/Civcraft/JukeAlert/pull/3.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#event-176001491.
rourke750
commented
9 years ago I fixed it but I misread the code. I merged back though. On Oct 8, 2014 9:08 PM, "ttk2" notifications@github.com wrote:
this should go in to the changelog, so why merge it early roruke?
On Wed, Oct 8, 2014 at 3:38 PM, rourke750 notifications@github.com wrote:
Merged #3 https://github.com/Civcraft/JukeAlert/pull/3.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#event-176001491.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#issuecomment-58451074.
ttk2
commented
9 years ago so its not in the jenkins builds now?
On Wed, Oct 8, 2014 at 8:19 PM, rourke750 notifications@github.com wrote:
I fixed it but I misread the code. I merged back though. On Oct 8, 2014 9:08 PM, "ttk2" notifications@github.com wrote:
this should go in to the changelog, so why merge it early roruke?
On Wed, Oct 8, 2014 at 3:38 PM, rourke750 notifications@github.com wrote:
Merged #3 https://github.com/Civcraft/JukeAlert/pull/3.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#event-176001491.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#issuecomment-58451074.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#issuecomment-58451780.
rourke750
commented
9 years ago Yes On Oct 9, 2014 9:13 PM, "ttk2" notifications@github.com wrote:
so its not in the jenkins builds now?
On Wed, Oct 8, 2014 at 8:19 PM, rourke750 notifications@github.com wrote:
I fixed it but I misread the code. I merged back though. On Oct 8, 2014 9:08 PM, "ttk2" notifications@github.com wrote:
this should go in to the changelog, so why merge it early roruke?
On Wed, Oct 8, 2014 at 3:38 PM, rourke750 notifications@github.com wrote:
Merged #3 https://github.com/Civcraft/JukeAlert/pull/3.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#event-176001491.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#issuecomment-58451074.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#issuecomment-58451780.
— Reply to this email directly or view it on GitHub https://github.com/Civcraft/JukeAlert/pull/3#issuecomment-58600928.
With the recent few patches / intended patches it has all been making defending easier. Also, with Group notifications for entries having no Counter-play except waiting till everyone is offline, which is hard with massive snitch networks with dozens of people added to it, who all live in different time zones.
So, for some there is never a time where someone is not online to see the notification.
Here is something to help balance a bit, if the user has an invisibility potion active it will not notify the group about an entry, it still will notify for log-ins and log-outs. It will also still record it to the Database if it shouldLog.
I also fixed the inconsistency in the indentation. So it is going to say I change a lot more than I actually did, check on line: https://github.com/KillerSmurf/JukeAlert/compare/Civcraft:master...master#diff-4a1d427f499cb9ee50b9bb8aab9274b9R347