FIX pipe_output / mutate naming convention and docstrings #1198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Docstring raised Pytest escape error due to /* so we wrap it in quotes.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 84b7b2a in 21 seconds
More details
- Looked at
65lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/function_modifiers/test_macros.py:838
- Draft comment:
Update the test to reflect the new naming convention for the first node in thepipe_outputchain. It should beresult_from_downstream_function.with_rawinstead ofresult_from_downstream_function_raw. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change made in the diff, specifically the renaming of a node in the test. The change has been implemented in the code, so the comment is not necessary as it describes an action that has already been completed.
The comment might be useful for historical context or for other developers to understand why the change was made, but it doesn't suggest a new action to be taken.
The purpose of the review is to identify actionable comments, and since the change has already been made, the comment does not serve an actionable purpose.
The comment should be deleted because it describes a change that has already been implemented and does not suggest any further action.
Workflow ID: wflow_vpMJjWLJbICwbI7q
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -1299,7 +1299,8 @@ def transform_node( | |||
| else: | |||
| _namespace = self.namespace | |||
|
|
|||
| original_node = node_.copy_with(name=f"{node_.name}_raw") | |||
| # We pick a reserved prefix that ovoids clashes with user defined functions / nodes | |||
| original_node = node_.copy_with(name=f"{node_.name}.with_raw") | |||
|
|
|||
| def __identity(foo: Any) -> Any: | |||
There was a problem hiding this comment.
The docstring mentions renaming the original function to function_name_raw, but the code now uses function_name.with_raw. Please update the docstring to reflect this change.
| @@ -1299,7 +1299,8 @@ def transform_node( | |||
| else: | |||
| _namespace = self.namespace | |||
|
|
|||
| original_node = node_.copy_with(name=f"{node_.name}_raw") | |||
| # We pick a reserved prefix that ovoids clashes with user defined functions / nodes | |||
| original_node = node_.copy_with(name=f"{node_.name}.with_raw") | |||
There was a problem hiding this comment.
Think this should be .raw instead of .with_raw
Current name convention is very prone to name clashes with user naming. We assign the same naming convention using namespace.raw to indicate the first node.
elijahbenizzy
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The naming convention for the first node in the
pipe_outputchain can potentially conflict with user-defined nodes. For example:would lead to
features_rawwhich is a common user naming convention. This makes it harder to have the conflict by following the same naming convention as the rest of the chained nodes.Changes
foo.with_rawmutate.__init__to avoid escape key errors in pytest.How I tested this
Notes
The docstring issue is currently only tracked on slack: https://hamilton-opensource.slack.com/archives/C03M34FM058/p1729431573025409
Checklist