A new method to format only with provided formats. #269
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AmountFormats
participant TestAmountFormats
User->>AmountFormats: call wordBasedWithFormats(period, duration, formats, locale)
AmountFormats-->>User: returns formatted string
User->>TestAmountFormats: run test_wordBasedWithFormats()
TestAmountFormats->>AmountFormats: call wordBasedWithFormats() with test data
AmountFormats-->>TestAmountFormats: returns expected results
TestAmountFormats-->>User: test results
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Any update on this one? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/java/org/threeten/extra/TestAmountFormats.java (1)
162-180: LGTM: New data provider added for wordBasedWithFormats tests.The
period_duration_wordBasedWithFormats()method provides comprehensive test data for the newwordBasedWithFormatsfunctionality. It covers various scenarios with different periods, durations, format lists, locales, and expected outputs.Consider adding a test case with an empty format list to ensure the method handles this edge case correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/java/org/threeten/extra/AmountFormats.java (3 hunks)
- src/test/java/org/threeten/extra/TestAmountFormats.java (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
src/main/java/org/threeten/extra/AmountFormats.java (3)
Line range hint
1-863: Overall assessment of changes to AmountFormats.javaThe changes to this file enhance the flexibility and customization options of the AmountFormats class. The new
wordBasedWithFormatsmethod and the increased visibility of constants provide users with more control over formatting options. The implementation is consistent with the existing codebase and follows good coding practices.Some minor improvements have been suggested, such as additional validation for the new method. It's also important to consider the long-term implications of making the constants public.
Please ensure that these changes are well-documented in the class and module level documentation, and that any public API changes are reflected in the project's changelog or release notes.
302-363: New method provides flexible word-based formattingThe new
wordBasedWithFormatsmethod enhances the class's functionality by allowing custom formatting of time units. This addition aligns well with the class's purpose and provides users with more control over the output format.Some observations and suggestions:
Null checks and validation:
The method properly checks for null parameters, which is good practice. Consider adding validation for theformatslist to ensure it's not empty.Time unit calculations:
The logic for calculating weeks and days is consistent with other methods in the class, which maintains coherence in the codebase.Use of streams:
The method uses streams and functional programming concepts, which is modern and concise. However, for very large lists of formats, this might have a slight performance impact. Consider benchmarking with large format lists to ensure performance meets expectations.Consistency:
The method follows the pattern of otherwordBasedmethods in the class, which is good for maintainability and understanding.Reusability:
The method reuses theWordBasedclass, promoting code reuse and consistency.Consider adding a check for an empty
formatslist:public static String wordBasedWithFormats(Period period, Duration duration, List<String> formats, Locale locale) { Objects.requireNonNull(period, "period must not be null"); Objects.requireNonNull(duration, "duration must not be null"); Objects.requireNonNull(formats, "formats must not be null"); + if (formats.isEmpty()) { + throw new IllegalArgumentException("formats list must not be empty"); + } Objects.requireNonNull(locale, "locale must not be null"); // ... rest of the method }To ensure the new method is properly integrated and doesn't introduce any regressions, run the following script:
#!/bin/bash # Check for usage of the new method and potential test coverage rg --type java "wordBasedWithFormats" -g '!AmountFormats.java'
95-123: Consider the implications of changing constant visibility.The visibility of several constants (WORDBASED_YEAR, WORDBASED_MONTH, etc.) has been changed from private to public. While this allows for external use and customization, it may impact encapsulation and make future modifications more challenging. Ensure that this change aligns with the library's design principles and future maintainability goals.
To verify the usage of these constants, run the following script:
src/test/java/org/threeten/extra/TestAmountFormats.java (3)
40-41: LGTM: New imports added for Arrays and List.These imports are necessary for the new test methods that use lists of format strings in the
wordBasedWithFormatsfunctionality.
188-192: LGTM: New test method added for wordBasedWithFormats functionality.The
test_wordBasedWithFormatsmethod is a well-structured parameterized test that uses the new data provider to test theAmountFormats.wordBasedWithFormatsmethod with various inputs. This ensures comprehensive testing of the new functionality.
Line range hint
40-193: Overall assessment: Comprehensive test coverage for new functionality.The changes in this file significantly enhance the test suite for the
AmountFormatsclass by adding support for the newwordBasedWithFormatsfunctionality. The additions include:
- New imports for
ArraysandList.- A new data provider method
period_duration_wordBasedWithFormats()with various test cases.- A new parameterized test method
test_wordBasedWithFormats.These changes ensure thorough testing of the new feature across different scenarios, time units, and locales. The code style and structure are consistent with the existing codebase, maintaining good readability and maintainability.
To ensure that the new
wordBasedWithFormatsmethod is properly implemented in theAmountFormatsclass, please run the following command:
You don't need to change LocalDateTime ld1 = ...;
LocalDateTime ld2 = ...;
Duration d = Duration.between(ld1, ld2);
PeriodDuration pd = PeriodDuration.of(d);
String format = AmountFormats.wordBased(pd.getPeriod(), pd.getDuration().truncatedTo(ChronoUnit.MINUTES), Locale.ENGLISH);It's impossible to fulfill your need for any As you can see above, it is sufficient to compute duration between 2 temporals, normalize it using However, to make it simpler, it would be worth considering adding these methods:
Edit: On second thought, I'm not sure about |
|
With those 2 new methods one could write something like: LocalDateTime ld1 = ...;
LocalDateTime ld2 = ...;
Duration d = Duration.between(ld1, ld2);
PeriodDuration pd = PeriodDuration.of(d);
String format = AmountFormats.wordBased(pd.truncatedTo(ChronoUnit.MINUTES), Locale.ENGLISH);But... see edit in previous comment. |
I'm having two instances of
LocalDateTimeand I want to get the time it has passed between them, but I only need days, hours and minutes.For this reason I have created a new method that allows users to specify which formats they want included in the final result.
Summary by CodeRabbit
New Features
Bug Fixes
AmountFormatsclass to ensure accurate functionality across various formats and locales.