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

Add support for multiple comments on the same Range #459

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Ivanov1ch
Copy link
Contributor

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]]. Adjusted comments.py, the web API, and the frontend to account for this. Also adjusted test_comments.py for more coverage on the new format. The interface for adding a comment in comments.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 a DeleteCommentModifierConfig with the comment_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

…ty with old Optional[Range] comment type. Update test_comments.py for new Comments/CommentModifier interfaces. All comments tests passing.
@Ivanov1ch
Copy link
Contributor Author

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.

@rbs-jacob rbs-jacob self-requested a review April 23, 2024 16:43
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

Successfully merging this pull request may close these issues.

None yet

1 participant