-
Notifications
You must be signed in to change notification settings - Fork 52
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
Feature/export #213
Feature/export #213
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.
פיצ'ר חשוב ומגניב
app/internal/agenda_events.py
Outdated
"""filter events by a time frame.""" | ||
|
||
yield from ( | ||
event for event in events | ||
if start <= event.start.date() <= end | ||
if ( |
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.
למה לא לצמצם את הפונקציות האלה לאחת בעזרת ארגומנט ברירת מחדל?
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 understand what @sagizaidor meant, but I do think it's actually a good idea to export this long statement to a separate function :)
tests/event_fixture.py
Outdated
@@ -51,8 +51,8 @@ def yesterday_event(sender: User, session: Session) -> Event: | |||
return create_event( | |||
db=session, | |||
title='event 3', | |||
start=today_date - timedelta(hours=8), | |||
end=today_date, | |||
start=today_date - timedelta(days=1, hours=8), |
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.
לא בטוח במאה אחוז במה שאני אומר - אבל לשנות פיקסטור שעוד בדיקות משתמשות בו מרגיש לי לא נכון.
מניח שעברת את כל הבדיקות אם עשית את זה וזה עובד. אבל בכל זאת תפס את תשומת ליבי
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.
Very good job! We have a bit of additional work to do :)
app/internal/agenda_events.py
Outdated
"""filter events by a time frame.""" | ||
|
||
yield from ( | ||
event for event in events | ||
if start <= event.start.date() <= end | ||
if ( |
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 understand what @sagizaidor meant, but I do think it's actually a good idea to export this long statement to a separate function :)
app/internal/agenda_events.py
Outdated
"""filter events by a time frame.""" | ||
|
||
yield from ( | ||
event for event in events |
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.
Probably can be in a single line(?)
app/routers/event.py
Outdated
def get_attendees_email(session: Session, event: Event): | ||
return (session.query(User.email).join(UserEvent) | ||
.filter(UserEvent.events == event).all()) | ||
# return (session.query(User.email).join(UserEvent) |
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.
Commented code
app/routers/share.py
Outdated
@@ -55,6 +55,7 @@ def send_in_app_invitation( | |||
session.add(Invitation(recipient=recipient, event=event)) | |||
|
|||
else: | |||
print(recipient.email, event.owner.email) |
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 logger instead :)
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.
my bad, it was there only for debugging
Codecov Report
@@ Coverage Diff @@
## develop #213 +/- ##
===========================================
+ Coverage 99.04% 99.06% +0.01%
===========================================
Files 50 51 +1
Lines 2310 2350 +40
===========================================
+ Hits 2288 2328 +40
Misses 22 22
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.
Great job! Probably near the end of it. Added few insights.
start: Union[None, date] = None, | ||
end: Union[None, date] = None, | ||
) -> Iterator[Event]: | ||
"""Returns all events in a time frame. |
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.
Blank line after this line
app/internal/agenda_events.py
Outdated
) | ||
for example: | ||
if start_date = None and end_date = datetime.now().date, | ||
then the function will return all events that ends before end_date.""" |
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.
When closing the docstring, """
should be in a separate line
app/internal/agenda_events.py
Outdated
then the function will return all events that ends before end_date.""" | ||
|
||
for event in events: | ||
if start and end: |
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.
Outside the for, define:
start = start or datetime.date.min
end = end or datetime.date.max
Inside the for, use:
if start <= event.start.date() <= end:
yield event
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 advice, thanks!
app/internal/agenda_events.py
Outdated
user_id: int, db: Session | ||
) -> Iterator[Event]: | ||
"""Yields all user's events in a time frame.""" | ||
|
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.
Prefer not having a blank line after function's docstring
app/internal/export.py
Outdated
"""Creates an ical event, | ||
and adds the event information""" |
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.
Join to a single line
app/internal/export.py
Outdated
def create_ical_event(user_event): | ||
"""Creates an ical event, | ||
and adds the event information""" | ||
|
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.
Remove the blank line here
Export the uses calendar with one click!