later icon indicating copy to clipboard operation
later copied to clipboard

Feat: Canceling exceptions

Open ilanbiala opened this issue 10 years ago • 24 comments

This is a WIP of being able to cancel exceptions through the use of a String tag. @bunkat once this is ready to review with all the proper tests, I'll let you know. I'm creating the PR now so anyone can suggest changes if they come up with any ideas.

ilanbiala avatar Feb 05 '15 04:02 ilanbiala

@bunkat how can I keep track of when a time period is called for an exception and not a schedule? The issue I'm facing right now is that I would have to check in every function whether a variable that keeps track of whether an exception is being created is true or not to decide whether to set the value in the object of exception tags to exceptions.

ilanbiala avatar Feb 06 '15 04:02 ilanbiala

A time period is considered valid when it is valid for all schedules and invalid for all exceptions. There isn't really a concept of time period being called for an exception. Though maybe I'm not understanding the question.

If you are trying to tag exceptions so that you can find them later, you'll need to tag them as you add them (by updating the recur() functions or manually placing a tag in the appropriate exception schedule object). For example, if you wanted to tag an exception as a holiday you would tag it when you enter it and if you wanted to turn it off later, you would just search the exceptions for things with that tag and then remove them.

bunkat avatar Feb 06 '15 04:02 bunkat

@bunkat I got all the tests I wrote passing and I even tested the code a little bit to see if it's pretty easy to work with. The functionality is all there and I didn't modify anything other than .recur() since that's what I use. Some of the tests that you or someone else wrote seem to be failing when I run it locally, and it seems that they are completely unrelated to mine. Do you know why some of the tests originally in Later aren't passing?

ilanbiala avatar Feb 11 '15 03:02 ilanbiala

@bunkat if you take a look at my first commit in this PR, you'll see that I added an if statement that does nothing, and even then tests weren't passing. I'm pretty sure none of the tests started failing as a result of the changes I made.

ilanbiala avatar Feb 11 '15 03:02 ilanbiala

@bunkat what should be done? Should we merge this in and then fix the failing tests in master?

ilanbiala avatar Feb 12 '15 00:02 ilanbiala

By the way, since a composite exception schedule is created using .and() and not .and().except() I had to add a the composite tag functionality into .and() if you're wondering why I did that. Let me know if the sample code in the tests could be better and I'll try to come up with a better way.

ilanbiala avatar Feb 12 '15 02:02 ilanbiala

@bunkat have you had a chance to look over this PR?

ilanbiala avatar Feb 12 '15 17:02 ilanbiala

Sorry, I'm quite busy at the moment and probably won't get a chance to look at this for a bit. Appreciate the PR and I'll get to it as soon as I can.

bunkat avatar Feb 12 '15 18:02 bunkat

@bunkat if you have any questions about the PR, feel free to ask me so that we can get this merged in as quickly as possible for others to use.

ilanbiala avatar Feb 14 '15 14:02 ilanbiala

@bunkat let me know if everything here is good so I can squash this PR down to 1 commit to make the commit log a little bit cleaner.

ilanbiala avatar Feb 15 '15 16:02 ilanbiala

@bunkat is there any way you can take a look at this PR and test out the functionality I added in? I'd like to use this functionality for another project.

ilanbiala avatar Feb 16 '15 19:02 ilanbiala

You don't need to wait for me to be able to use this functionality. Just create a build out of your repository and go for it.

bunkat avatar Feb 16 '15 19:02 bunkat

I'm referring to the server-side, because I'm looking to use this through the node package and not have to make lots of changes later because of monkey patching it now. Is there any issue with the code I added that prevents you from merging it in?

ilanbiala avatar Feb 16 '15 20:02 ilanbiala

Any update on getting this PR merged in?

ilanbiala avatar Feb 20 '15 23:02 ilanbiala

@bunkat do you have any questions about the functionality?

ilanbiala avatar Feb 24 '15 03:02 ilanbiala

Really sorry, I do try and keep up with things better but unfortunately I'm swamped right now and I'm not sure when I'm going to have a chance to get these PRs reviewed and merged in. I wouldn't wait on me, just use it as a local module instead of via npm. Once it gets merged it will be easy enough to move it over. I'll try and get to all these next month.

bunkat avatar Feb 24 '15 09:02 bunkat

@bunkat just let me know when I should squash the commits so that you can merge.

ilanbiala avatar Mar 03 '15 04:03 ilanbiala

@bunkat have you had a chance to take a look at this?

ilanbiala avatar Mar 21 '15 18:03 ilanbiala

@bunkat can this be merged in and released as part of Later 1.2.0?

ilanbiala avatar Mar 29 '15 00:03 ilanbiala

@bunkat everything coming along ok? It's been quite some time and I just wanted to see if you can still merge this PR.

ilanbiala avatar Apr 03 '15 16:04 ilanbiala

@bunkat any issue with merging this?

ilanbiala avatar May 14 '15 23:05 ilanbiala

@bunkat we should be testing against these new versions now that many people use them, can we merge this PR in?

ilanbiala avatar Jun 01 '15 16:06 ilanbiala

@bunkat still waiting on this. Can you merge it in?

ilanbiala avatar Aug 18 '15 11:08 ilanbiala

Per my work with Bree, I have an updated fork of this package called @breejs/later. See https://github.com/breejs/later if you would like to file this PR there.

niftylettuce avatar Aug 17 '20 14:08 niftylettuce