-
Notifications
You must be signed in to change notification settings - Fork 52
add: world_clock.py and test_world_clock.py #222
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
Conversation
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.
That's a good start! let's have the first improvement cycle :)
app/routers/world_clock.py
Outdated
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.
We shouldn't manage our own database of timezones. Please use pytz.all_timezones
or something familiar :)
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 have looked into several modules in Python, including pytz, and I can not see how they help here. I have invested a lot of time and energy in writing a code that supports large amount(!) of cities and places and not just the "timezone places" themselves.
the "Mini DB" I've created is not just a list of timezones - it maps timezones to countries and subcountries and crucial for the core of my code - Ability to search the equivalent time of a custom city or place.
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.
Added few more comments :)
Hi :)
Can you please response to the 2 comments I wrote above?
Codecov Report
@@ Coverage Diff @@ ## develop #222 +/- ## =========================================== - Coverage 99.38% 95.54% -3.84% =========================================== Files 37 80 +43 Lines 1456 3727 +2271 =========================================== + Hits 1447 3561 +2114 - Misses 9 166 +157
Continue to review full report at Codecov.
|
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.
Hi, I've added further insights.
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.
Great start, just few things before we can merge it
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.
Great job, please take a look at the insights :)
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.
Cache this function to prevent high loading times
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 did my best to do so by using the decorator @functools.lru_cache
but I always get runtime errors with the message: "cannot reuse already awaited coroutine". The truth is I'm pretty desperate and do not know how to solve this. I probably won't have the time to work on the code again by the end of the submission deadline - Can you consider giving up this matter or help me here? I have worked so hard...
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.
Have you tried async-lru?
app/internal/world_clock.py
Outdated
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.
Use git precommit hooks to fix this formatting
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 don't see it. this note has already been resolved and I see the correct formmating without the \
char.
This note is also "Outdated". Can you check again? :)
app/resources/country-continent.json
Outdated
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.
Keys should be lowercase, reformat in a human readable way.
Also, is there really no python module that does this? I find this very very unlikely...
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.
Have you tried async-lru?
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.
Cache the return value, reading and parsing JSON every time is expensive
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.
Cache the return value, reading and parsing JSON every time is expensive
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.
Cache this
- I tried to use both
async-lru
, for the asynchronous function, andfunctools.lru_cache
, for uploading the files,
and many tests fail suddenly forRuntimeError: Event loop is closed
and174 Http Error
errors.
I tried everything and I am lost. - Also, my code is after the prehook fixes, so I do not know what I have left to fix ...
There are few comments that aren't related to caching. Fix them and I'll consider this as a ticket :)
No description provided.