-
Notifications
You must be signed in to change notification settings - Fork 923
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
increase max file uploading number #1794
Merged
Merged
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to e13803c in 2 minutes and 49 seconds
More details
- Looked at
33
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. skyvern/constants.py:22
- Draft comment:
Consider adding a brief comment on the purpose of MAX_UPLOAD_FILE_COUNT. It now controls the max number of files for uploading. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The constant's name MAX_UPLOAD_FILE_COUNT is quite self-descriptive. The comment's suggestion to add documentation that says "controls the max number of files for uploading" would be redundant with the variable name. Most other constants in this file don't have explanatory comments unless they need extra context (like timeouts having duration units specified).
Perhaps there's some important nuance about this limit that future developers need to know? Maybe the number 50 has some specific reasoning that should be documented?
While there might be reasons for the specific value of 50, the purpose of the constant itself is crystal clear from its name. If the value's reasoning needs documentation, that would be a different comment.
The comment should be deleted as it suggests adding documentation that would be redundant with the self-documenting variable name.
2. skyvern/forge/sdk/workflow/models/block.py:1433
- Draft comment:
Ensure that allowing exactly MAX_UPLOAD_FILE_COUNT files is intended since the condition uses '>' (i.e. count == MAX_UPLOAD_FILE_COUNT is allowed). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is speculative - it's asking whether allowing exactly MAX_UPLOAD_FILE_COUNT files is intended. The current code has a clear behavior - it allows up to MAX_UPLOAD_FILE_COUNT files and rejects more than that. There's no strong evidence that this is incorrect. The comment is asking for confirmation rather than pointing out a clear issue.
The comment could be right - maybe we should be more restrictive and reject when count equals the max. But without more context about the requirements, we can't be sure.
The current code has a clear, reasonable behavior. Without evidence that this behavior is wrong, we shouldn't ask the author to second-guess their implementation.
Delete the comment. It's asking for confirmation rather than pointing out a clear issue, and there's no strong evidence that the current behavior is incorrect.
3. skyvern/constants.py:22
- Draft comment:
Consider making MAX_UPLOAD_FILE_COUNT configurable (e.g. via environment or settings) if the limit might need adjustment in different deployments. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is speculative - it suggests making something configurable "if" it might need adjustment. There's no strong evidence that this value needs to be configurable. Other similar constants in the file are hardcoded. Making constants configurable adds complexity and should only be done when there's a clear need.
Perhaps this is a critical limit that different deployments really do need to configure differently. Maybe there's context in other files showing this need.
Without clear evidence that this needs to be configurable, adding configuration adds unnecessary complexity. If a need arises later, it can be made configurable then.
Delete the comment. It's speculative and suggests adding complexity without clear evidence of need.
4. skyvern/forge/sdk/workflow/models/block.py:1433
- Draft comment:
Update the error message to include the MAX_UPLOAD_FILE_COUNT value (e.g. using f-string) for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
5. skyvern/constants.py:12
- Draft comment:
Typographical suggestion: The comments on lines 12-14 use 'minute' in the singular (e.g., '3 minute', '2 minute', '10 minute'). Consider updating them to 'minutes' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. skyvern/forge/sdk/workflow/models/block.py:743
- Draft comment:
Typographical error: The variable 'last_ouput' appears to be misspelled. It should likely be 'last_output' to enhance clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_SIEJQOhZXvJZImGz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
wintonzheng
approved these changes
Feb 20, 2025
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Important
Increase max upload file count from 25 to 50 by adding
MAX_UPLOAD_FILE_COUNT
constant and updating logic inblock.py
.block.py
.ValueError
if file count exceedsMAX_UPLOAD_FILE_COUNT
.MAX_UPLOAD_FILE_COUNT = 50
toconstants.py
.MAX_UPLOAD_FILE_COUNT
inblock.py
.This description was created by
for e13803c. It will automatically update as commits are pushed.