Open Ivanov1ch opened 2 months ago
A question that remains is about backwards compatibility and, of course, the hardest question of all - naming. In the current state, DeleteCommentModifierConfig
is defined as follows:
@dataclass
class DeleteCommentModifierConfig(ComponentConfig):
"""
comment_range: Tuple[Optional[Range], comment_text=Optional[str]]
If comment_text is provided, deletes the matching comment with the same Optional[Range]
If comment_text is None, deletes ALL comments with the same Optional[Range]
"""
comment_range: Tuple[Optional[Range], Optional[str]]
def __post_init__(self):
# Ensure there's always a two-element Tuple
if type(self.comment_range) == tuple:
# New format
self.comment_range = (*self.comment_range, None)[:2]
elif type(self.comment_range) == Range:
# Old format: Range
self.comment_range = (self.comment_range, None)
else:
# Old format: None (no Range provided)
self.comment_range = (None, None)
So the field comment_range
retained the same name as the current definition of the class:
@dataclass
class DeleteCommentModifierConfig(ComponentConfig):
comment_range: Optional[Range]
Which ensures that any old code that would run something like this still works:
await executable_resource.run(
DeleteCommentModifier,
DeleteCommentModifierConfig(comment_range=Range(0, 2)),
)
However, the name comment_range
makes less sense now as this is more than just an Optional[Range]
. Another option could be to leave comment_range: Optional[Range]
and add a second field named something like comments_to_delete: Tuple[Optional[Range], Optional[str]]
and use the post-init hook to populate it from comment_range
if it is still used.
Any input would be appreciated.
One sentence summary of this PR (This should go in the CHANGELOG!) Added support for multiple comments on the same Range Link to Related Issue(s)
458
Please describe the changes in your request. Refactored
CommentsAttributes.comments
to from the current comments: Dict[Optional[Range], str] to comments: Dict[Optional[Range], List[str]]. Adjustedcomments.py
, the web API, and the frontend to account for this. Also adjustedtest_comments.py
for more coverage on the new format. The interface for adding a comment incomments.py
(AddCommentModifierConfig
) has not been changed, but the interface for deleting a comment (DeleteCommentModifierConfig
) has been.DeleteCommentModifierConfig
has been modified in a way that should remain backwards compatibility - creating aDeleteCommentModifierConfig
with thecomment_range
of the old type,Optional[Range]
will still work with the new type:Tuple[Optional[Range], Optional[str]]
, as the post-init hook will automatically convert it.test_comments.py
has also been adjusted and expanded. Anyone you think should look at this, specifically? @rbs-jacob