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
Cleanup deployment-start-time feature branch, add tests #7928
Merged
serinamarie
merged 38 commits into
deployment-start-time
from
deployment-start-time-continued
Dec 28, 2022
Merged
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
87eb843
Update docstring
serinamarie b108a7d
Cleanup start-in start-at param
serinamarie 97d41d6
Move start_in, start_at below other params
serinamarie 7ab9503
Move human_readable_dt_diff assigning
serinamarie 637ad4a
Add test template
serinamarie bd0f1c3
Rename test
serinamarie ab00c3e
Rename tests
serinamarie 2016455
Start implementing tests for scheduling behavior
zanieb 3a709d3
Add test for invalid input
zanieb e5c73f0
Add more tests
serinamarie 58c2217
Fix tests
serinamarie 1c1f555
Rename long var name
serinamarie 4ff7d46
Handle unexpected exceptions
serinamarie 259bec9
Add more tests
serinamarie 3eeeb55
Add exception handling when user input includes tz
serinamarie 9370e2a
Update test_start_at_option_schedules_flow_run_in_future
serinamarie 23d36dd
Modify tests
serinamarie be6ff76
Accommodate timezones
serinamarie 5aa10f3
Some cleanup
serinamarie 180fd45
Remove diff_for_humans because of 1 second lag
serinamarie 8a77b92
Remove 2nd parsing for display, add future pref in dateparsing
serinamarie 6b4f94d
add test, change expected values due to future dateparser setting
serinamarie 1f9f23e
Fix tests, hopefully
serinamarie 927deba
Remove 2nd calculation of local tz
serinamarie 74503df
Cleanup tests
serinamarie 55bfce4
Cleanup
serinamarie d089ddb
bump dateparser version
serinamarie 3ef506a
Fix tests
serinamarie 104e72b
rename tests, test var
serinamarie 8912f52
Modify print statements, docstrings
serinamarie 4b80352
Put parsing logic for start_time params in a helper function
serinamarie 3320144
Rename helper function
serinamarie 07c7cb7
Update src/prefect/cli/deployment.py
serinamarie b205827
Update src/prefect/cli/deployment.py
serinamarie 02b9f14
Remove helper function
serinamarie 358adb7
Add test to document start-in behavior
serinamarie eaa14c5
Correct test
serinamarie 44f03b4
Modify exit error & corresponding test
serinamarie File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
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.
Since the above exits if both are set you can just do
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.
Do you mean
else:
being wherestart_in
is None andstart_as
in None? Because there is some logic beneath that specific tostart_in
/start_at
for assigningstart_time_parsed
,scheduled_start_time
, andhuman_dt_diff
.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.
Yeah I think you can do
else
instead ofelif start_in is None and start_at is None:
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.
I'm curious too what this would look like without having a helper function for the raw parsing logic.
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.
I think the helper function is a bit harder to read than the initial implementation, especially since it does more than one thing (parses the date time and generates a formatted diff). I don't have strong feelings here though.
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.
(unresolving for visibility; does not mean it needs to be addressed)