adding comment creation support for domain layer & base repo #409
Conversation
|
|
rubicon_ml/domain/mixin.py
Outdated
| comments : List[str] | ||
| A list of string comments to add to the domain model. | ||
| """ | ||
| self.comments = list(set(self.comments).union(set(comments))) |
There was a problem hiding this comment.
this was originally in tags because we didn't want duplicate tags. comments are pretty unstructured, so I don't think its necessary to guard against duplicates here. let's just do self.comments.extend(comments)
There was a problem hiding this comment.
I'll remove it, but just for the sake of curiosity - is there ever a situation we would actually want duplicates? Even if the check isn't as necessary, does having it actually hurt anything?
There was a problem hiding this comment.
I could imagine multiple users having a conversation and there being multiple "yes"s, "no"s. etc. but then again we'd probably want to make these more robust (like including authors) if we really wanted to support conversations like that
also, the list/union/list conversion is O(n) so it could become slow. but that'd require a lot of comments
|
|
||
| # ---------- Comments ---------- | ||
|
|
||
| def _get_comment_metadata_root( |
There was a problem hiding this comment.
is there any difference between this and _get_tag_metadata_root? the metadata root is gonna be the same - you're just changing "tag" in the filename to "comment", which happens in the function you added below this one. if so, you can just have this function call _get_tag_metadata_root
There was a problem hiding this comment.
Yeah, there's no real difference. I just thought it might be good form to add a comment specific one in case we wanted to differentiate between the two more in the future, but I think it's fine to remove now.
There was a problem hiding this comment.
yeah that definitely makes sense, and I could see the comments becoming a bit more complicated. if we just do something like
def _get_comment_metadata_root(args):
# comments and tags are currently written to the same root with a different filename
return self._get_tag_metadata_root(args)
we can have the best of both worlds - a new function available if it needs to change and reuse of the old one
| The name of the entity's type as returned by | ||
| `entity_cls.__class__.__name__`. | ||
| """ | ||
| comment_metadata_root = self._get_comment_metadata_root( |
There was a problem hiding this comment.
if my comment above is correct, you could just change this to _get_tag_metadata_root and leave a comment explaining that they're the same root path too. then just totally get rid of the above function. whichever is easier for you
| experiment = _create_experiment(repository) | ||
| repository.add_comments( | ||
| experiment.project_name, | ||
| ["wow"], |
There was a problem hiding this comment.
can we make this value a few words long? just wanna validate that its not splitting on spaces or anything like that
|
also not sure why the build isn't triggering. try rebasing off main and see if that fixes it |
|
looks like the linting failed. run |
What
How to Test
pytest tests