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
Hosting CLI: use http endpoint to return deploy milestones #2085
Conversation
@ElijahAhianyo could you help me try this PR for deploying an app? I think you ran into some issues https://discord.com/channels/1029853095527727165/1168071548817645599/1168090802740809778 I just switched out the websocket and replaced with http and retries. Let me know if it helps with this particular situation. There'll be a different PR for the overall slowness. |
Tried this fix out and it doesnt work for me, Will send the logs and details on discord |
Returns: | ||
The converted timestamp with timezone. | ||
""" | ||
try: |
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.
What causes this to raise an exception? Generally shouldn't catch bare Exceptions
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.
the conversion can raise TypeError, ValueError, let me change to the more specific exception types.
reflex/utils/hosting.py
Outdated
local_dt = datetime.fromisoformat(iso_timestamp).astimezone() | ||
local_dt = convert_to_local_time_with_tz(iso_timestamp) | ||
if local_dt is None: | ||
return iso_timestamp | ||
return local_dt.strftime("%Y-%m-%d %H:%M:%S.%f %Z") | ||
except Exception as ex: |
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.
Same here, can we catch a more specific error / do we know why we need the catch?
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.
Actually we don't need to catch here, it's already caught in the call to convert.
reflex/utils/hosting.py
Outdated
from_iso_timestamp = maybe_timestamp + timedelta(microseconds=1e5) | ||
else: | ||
console.error(f"Unable to parse timestamp {row['timestamp']}") | ||
raise ValueError |
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.
Should we specify an error in the ValueError? Also, do we want to break out of the loop here or just continue?
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.
Good catch, we should continue here instead.
90104f1
to
14b64cc
Compare
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features:
After these steps, you're ready to open a pull request.