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

Question about toDo #31

Closed
dernster opened this issue Sep 12, 2016 · 5 comments
Closed

Question about toDo #31

dernster opened this issue Sep 12, 2016 · 5 comments

Comments

@dernster
Copy link

Reviewing the public static void toDo(@Scope int scope, String tag) function specification it says "if it has already marked to do or been done within that scope then it will not be marked." but the actual implementation does not involve the THIS_APP_SESSION scope. I'm wondering why the implementation does not check if the tag was done within that scope? Maybe we should have a session start timestamp in order to do that... what do you think?

@jonfinerty
Copy link
Owner

Well spotted, looks like a bug.

That should probably check the sessionList if the THIS_APP_SESSION scope is provided and only add the tag if it doesn't exist in the list.

longer term, I'd like to change how the sessionList works and shift to how you described, so you have a session timestamp and it mirrors how the version scope check works.

Other work here is that toDo doesn't work with timestamps yet, which would make it nice and symmetric with beenDone

@jonfinerty
Copy link
Owner

@dernster I've just checked the code, we have a test around this. It's not obvious from the code (because as you've seen the implementation doesn't explicitly check THIS_APP_SESSION), but actually because we only mark a tag as todo if its either it's never been done before, or if the scope is VERSION and we last saw the tag before the last update.

As I said this could use a refactor :) but it should be functional (check here: https://github.com/jonfinerty/Once/blob/master/once/src/test/java/jonathanfinerty/once/ToDoTests.java#L59 for validation)

@jonfinerty
Copy link
Owner

@dernster I'll going to close this, if that's ok? I'll do the refactoring at some point, but outside of this issue as it's more an enhancement.

@dernster
Copy link
Author

@jonfinerty You are right! Thanks for clarifying that :)
It's ok to close this.

I was reviewing your code since I was thinking about implementing this library for Swift. Our Android team use this Once component a lot and for iOS there was nothing like this.
Finally did it! You can check out Ecno (Once in reverse :)). In the README file we have a reference to your library so I hope it's all fine.

Thanks!

@jonfinerty
Copy link
Owner

Amazing, thanks for the link, it's really cool someone is building upon something I built.

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

No branches or pull requests

2 participants