𖭦 Ability to override stage on enqueue #89
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to d3b859d in 29 seconds
More details
- Looked at
421lines of code in12files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. sdk/harambe/core.py:598
- Draft comment:
Consider handling unexpectedprevious_stagevalues more explicitly, perhaps by raising an exception or logging a warning, instead of defaulting to 'detail'. - Reason this comment was not posted:
Confidence changes required:50%
Theget_next_stagefunction should handle unexpectedprevious_stagevalues more gracefully. Currently, it defaults to 'detail', which might not be the best approach if the input is invalid.
2. sdk/harambe/observer.py:152
- Draft comment:
Ensure that theurlsproperty inInMemoryObserverand other observers reflects the updated tuple structure, including thestageparameter. - Reason this comment was not posted:
Confidence changes required:50%
Theon_queue_urlmethod inInMemoryObserverand other observers now includes astageparameter, but theurlsproperty inInMemoryObserveris not updated to reflect this change. This could lead to confusion or errors when accessingurls.
3. sdk/test/test_pagination.py:48
- Draft comment:
Consider using a validstagevalue instead ofNoneto ensure the test is meaningful and aligns with the intended use case. - Reason this comment was not posted:
Confidence changes required:50%
Thetest_stop_pagination_observer_duplicate_url_errortest function usesNonefor thestageparameter, which might not be the intended use case. It would be better to use a valid stage value to ensure the test is meaningful.
Workflow ID: wflow_I7gSZD95RrXfYaoZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3177496 in 23 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_9qjDgXqdS0iTpel7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d677ae5 in 29 seconds
More details
- Looked at
102lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. sdk/test/test_get_next_stage.py:29
- Draft comment:
The expected stage forNoneshould belistingto match theget_previous_stagefunction's behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential issue with the test case forget_previous_stage. If the function's intended behavior is indeed to mapNonetolisting, then the test case is incorrect. However, without additional context on the intended behavior ofget_previous_stage, it's difficult to confirm the comment's validity. The comment is about a change made in the diff, specifically the new test case forget_previous_stage.
I might be missing the intended behavior of theget_previous_stagefunction. The comment assumes a specific behavior without providing evidence from the code or documentation.
The comment could be valid if there is documentation or code elsewhere that specifies the expected behavior ofget_previous_stage. However, without such evidence, the comment remains speculative.
The comment is speculative without strong evidence. It should be deleted unless there is clear documentation or code indicating the expected behavior ofget_previous_stage.
Workflow ID: wflow_S7Bcw0vV4pLhi9tX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
awtkns
reviewed
Nov 20, 2024
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "previous_stage,expected_stage", |
There was a problem hiding this comment.
I don't think this should be introduced to harambe. The consumer of the observer should make this choice
awtkns
reviewed
Nov 20, 2024
| """ | ||
| context = context or {} | ||
| options = options or {} | ||
| stage = stage or get_next_stage(self._stage) |
There was a problem hiding this comment.
Would recommend leaving the getnextstage outside of harambe (separation of concerns)
| @@ -507,19 +510,23 @@ async def run_from_file( | |||
|
|
|||
| tracker = FileDataTracker(domain, stage) | |||
|
|
|||
| prev = "listing" if stage == "detail" else "category" | |||
There was a problem hiding this comment.
I think we should remove this run from file / stage stuff here too
Comment on lines
+605
to
+613
| def get_next_stage(previous_stage: Stage | None) -> Stage: | ||
| if previous_stage == "parent_category": | ||
| return "category" | ||
| if previous_stage == "category": | ||
| return "listing" | ||
| return "detail" | ||
|
|
||
|
|
||
| def get_previous_stage(stage: Stage | None) -> Stage: |
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.
Closes #APE-345
Important
Add stage override capability to
enqueue()in Harambe SDK with updated tests and version bump to 0.51.0.stageparameter toenqueue()incore.pyfor stage override.get_next_stage()incore.pyto determine the next stage.on_queue_url()inobserver.pyto acceptstageparameter.Stageintypes.py.test_get_next_stage.pyto testget_next_stage().test_e2e.py,test_pagination.py, andtest_sdk.pyforstageparameter.0.51.0inpyproject.tomlanduv.lockforharambe-coreandharambe-sdk.This description was created by
for d677ae5. It will automatically update as commits are pushed.