docs: document the empty state of the no-argument constructor - #4
Conversation
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Shift from defensive language (e.g. "not a failure", "intentional, not a failure: [...] deliberately" ) to direct, authoritative behavior documentation.
|
|
||
| ### Components without a value yet | ||
|
|
||
| `new RelativeTime()` writes no `datetime` attribute, so **it renders nothing** until `setDateTime` is called. This is intentional, not a failure: there is deliberately no default value, because defaulting to the current instant would display a time that is not the intended one, and would keep ticking away from it until the real value arrived. |
There was a problem hiding this comment.
Consider a rewrite:
- Clear & Technical
new RelativeTime()intentionally renders nothing untilsetDateTime()is called. Omitting a default datetime attribute prevents displaying an incorrect initial timestamp that drifts out of sync while awaiting the actual value.
- Direct & Punchy
By design,
new RelativeTime()stays empty on instantiation untilsetDateTime()runs. Defaulting to the current time would show inaccurate data that continuously ticks away from the intended target until the true value arrives.
- Concise (Single-Sentence)
new RelativeTime()deliberately renders nothing untilsetDateTime()is invoked to prevent showing a temporary default timestamp that drifts further out of sync before the real data arrives.
There was a problem hiding this comment.
Took option 1, with two changes. Dropped "intentionally", since option 3's "deliberately" is the same register the review flags. And kept "writes no datetime attribute", because that's the concrete thing a reader can check in the DOM, and all three options dropped it.
Result: "new RelativeTime() writes no datetime attribute and renders nothing until setDateTime is called. No default is applied: the current instant would be a value the caller did not choose, and it would keep ticking away from the intended one until that value arrives."
| /** | ||
| * Creates a component with no target datetime: nothing is rendered until {@link #setDateTime} is | ||
| * called. Use it when the value is not available at construction time, such as in component | ||
| * renderers ({@code new ComponentRenderer<>(RelativeTime::new, (rt, item) -> ...)}), | ||
| * asynchronously loaded data, or displays that start empty and are reset with {@link #clear()}. | ||
| * | ||
| * <p>No default is applied on purpose: defaulting to the current instant would display a time | ||
| * other than the intended one, and would keep ticking away from it. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Creates a component with no target datetime: nothing is rendered until {@link #setDateTime} is | |
| * called. Use it when the value is not available at construction time, such as in component | |
| * renderers ({@code new ComponentRenderer<>(RelativeTime::new, (rt, item) -> ...)}), | |
| * asynchronously loaded data, or displays that start empty and are reset with {@link #clear()}. | |
| * | |
| * <p>No default is applied on purpose: defaulting to the current instant would display a time | |
| * other than the intended one, and would keep ticking away from it. | |
| */ | |
| /** | |
| * Creates a relative time component without a target datetime. Renders nothing until | |
| * {@link #setDateTime} is called. | |
| * | |
| * <p>Use this constructor when the datetime is unavailable at creation, such as in | |
| * component renderers ({@code new ComponentRenderer<>(RelativeTime::new, ...)}), | |
| * asynchronous data loading, or views reset via {@link #clear()}. | |
| */ |
There was a problem hiding this comment.
Applied, with one addition. I kept a short paragraph on why there is no default, because that rationale is what issue #3 was about, and without it in the code the question comes back the next time someone reads the constructor. Reworded in the direct voice:
"No default datetime is applied. The current instant would be a value the caller did not choose, and it would keep ticking away from the intended one until that value arrives."
Also changed "views reset via clear()" to "displays", since clear() resets the component, not a view.
| * <p><b>Empty state.</b> A component with no target datetime renders nothing at all: the | ||
| * underlying element has no text to show. This is the documented behaviour of the | ||
| * {@linkplain #RelativeTime() no-argument constructor} and of {@link #clear()}, not a failure; see | ||
| * that constructor for when an empty component is the right starting point. |
There was a problem hiding this comment.
| * <p><b>Empty state.</b> A component with no target datetime renders nothing at all: the | |
| * underlying element has no text to show. This is the documented behaviour of the | |
| * {@linkplain #RelativeTime() no-argument constructor} and of {@link #clear()}, not a failure; see | |
| * that constructor for when an empty component is the right starting point. | |
| * <p><b>Empty state.</b> When no target datetime is configured, this component renders blank. | |
| * This is intentional for both the {@linkplain #RelativeTime() no-argument constructor} and {@link #clear()}. | |
| * See the constructor Javadoc for applicable use cases. |
github-project-automation
Bot
moved this from To Do
to In Progress
in Flowing Code Addons
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe changes document the empty state of ChangesRelativeTime empty-state behavior
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This documentation-only change clarifies the existing empty-state behavior and adds usage examples; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR documents the empty rendering behavior of RelativeTime(), including README guidance and constructor Javadoc, as required by issue Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Agreed. The wording was arguing against the issue instead of documenting the behavior. Removed "not a failure", "intentional" and "deliberately" from all four places: the two you flagged, plus |
github-project-automation
Bot
moved this from In Progress
to Pending release
in Flowing Code Addons
2 participants
docs: document the empty state of the no-argument constructor
new RelativeTime()renders nothing until setDateTime is called, which read as a broken component on first use. The behavior was only mentioned in SPECIFICATIONS.md, and the constructor Javadoc just said "Creates an empty component".PR includes the following documentation updates:
Close #3
Summary by CodeRabbit
RelativeTimecan be created without an initial date and remains empty until a date is provided.clear()returns the component to its empty state.