-
Notifications
You must be signed in to change notification settings - Fork 47
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
Use standard Java classes #77
Comments
Thanks for your suggestions. I've indeed already considered and planned some of these changes. Please bear in mind that the library is also being used in Android Projects, which means we can't use Java 8 API unless we drop support for everything prior to Android 8. Our OpenTasks app supports Android 4.4+. That being said, I do agree with your points and I'm not completely happy with the current design either. In particular I'd like to replace I do have some very concrete ideas, but I'm happy to discuss your changes. |
@dmfs I'm not super familiar with maintaining java libraries (as opposed to android-specific libraries), but one option for using the Java8 classes could be for this library to update to use the Java 8 APIs and then advise consuming Android applications to use the new Java 8 desugaring support bundled with the Android Gradle Plugin 4.0.0 mentioned here: Not 100% sure if there are any issues with that approach as I've only tried the deseugaring process with using Java 8 time directly rather than through another library, but it might be a viable option. |
@jdvp right, I guess with the advent of the new desugaring features, we no longer need to refrain from using Java 8 API. I'll give that a try. |
This library is excellent, thank you for creating it.
There are quite a few places where using standard Java classes may make it more usable.
RecurrenceRuleInterator -> This seems like it could implement Iterator which would then make it much easier to generate a Stream. Alternatively, RecurrenceRule could have a stream method which returned a stream.
Weekday -> There is already DayOfWeek. Could this be used instead?
DateTime -> This is similar to either LocalDate (all day floating), LocalDateTime (floating), ZonedDateTime (timezone). I can see it is convenient, internally, to have a single class that can represent any of these concepts but externally, RecurrenceRule could accept these more standard classes, and the recurrence rule could emit them, and the conversion to DateTime could happen internally.
These kinds of changes would allow the library to work more easily with other code which will already be using these classes.
I've wrapped the library but I'm wondering if you would find these changes useful in this library itself and if so, whether you would be open to some PRs?
The text was updated successfully, but these errors were encountered: