Skip to content
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

start_date_local doesn't include activity timezone #237

Open
SebTota opened this issue Jun 5, 2022 · 6 comments
Open

start_date_local doesn't include activity timezone #237

SebTota opened this issue Jun 5, 2022 · 6 comments
Labels
pending-response A label to track issues where we are waiting for a user response

Comments

@SebTota
Copy link

SebTota commented Jun 5, 2022

I recently ran into an issue when I was converting an activities start_date_local value from the local time to other time zones. I'm able to handle this locally with the following method, but I was wondering why this isn't built into the library. I can look into making this modification and creating a PR for it, but I wanted to check if this is expected behavior or not.

Here is the method I've been using to get around this:

def _get_activities(self, start: datetime, end: datetime) -> [Activity]:
    """
    Get the users activities between the specified date
    :param start: start time in UTC
    :param end: end time in UTC
    :return: a list of activities that fell between the specified dates
    """
    l: [Activity] = list(self.strava_util.get_activities(after=start, before=end))
    for a in l:
        a.start_date_local = pytz.timezone(str(a.timezone)).localize(a.start_date_local)
    return l
@jsamoocha
Copy link
Collaborator

I'm not sure what exactly you're trying to do, but maybe it helps to use the start_date (which is always in UTC) attribute instead of start_date_local?

@SebTota
Copy link
Author

SebTota commented Jun 6, 2022

My use case includes finding the start and end date and time local to the activity and then converting that to UTC. I could use the UTC date to start, but I would still need to convert it to the local timezone to find the start and end of the date local to the activity which I was just trying to use start_date_local for.

In general though this causes a pretty big issue if the user isn't aware of this. When I was testing on my local machine where the timezone was set to PST and the activity took place in the PST timezone, the behavior was as expected because (I'm assuming) the python library I was using to converts the datetime objects timezones took the local PC's timezone as the default one since the datetime object didn't have one specified. When I deployed these changes though to a machine that uses UTC as it's default timezone, I quickly realized I was getting really weird start and end datetimes for the activities coming in.

@jsamoocha
Copy link
Collaborator

To get the end date/time in UTC, you could add the activity's elapsed_time (which is already of timedelta type) to the start_date to give you timezone-aware start - and end times in UTC.

I agree it seems more consistent to make the start_date_local timezone-aware, just as start_date. However, retrospectively doing this may break a lot of existing code that depends on the current behavior.

@SebTota
Copy link
Author

SebTota commented Jun 6, 2022

I'm not looking to get the start and end date and time of the activity, but rather the start and end of the entire day the activity took place.

You do bring up a good point about breaking existing functionality, but I do think that adding a small comment or something may help people not have to spend time debugging their code to find this issue in the future.

Edit: I know this would break the 1:1 mapping of variables in the Activity object to what is returned from the API, but even adding a localized_start_date and keeping local_start_date as is would be a stop gap for this. I don't mean to push as at this point this makes no difference to me since I just implemented my own function to solve this issue for me, but this took me hours of debugging to even figure out what was going wrong so I just want to make sure people using this library in the future don't have this same issue.

@lwasser
Copy link
Collaborator

lwasser commented Feb 12, 2024

hi there @SebTota 👋 it looks like this issue has been quiet for some time. i'm going through and trying to fix / close / cleanup old issues. i'm not actually able to find the repo or the link above seems to be broken at this point.

But is this still an issue? If it would be as simple as a small docstring update to identify the point of confusion (if that still exists) we could explore that.

Please respond and let us know if this is still an ongoing challenge or if you feel like it's resolved.

if i don't hear from you within a week i'll close it. but we can always reopen! i'm just trying to ensure that we are focusing our collective time on current stravalib issues!

@lwasser lwasser added the pending-response A label to track issues where we are waiting for a user response label Feb 12, 2024
@SebTota
Copy link
Author

SebTota commented Mar 18, 2024

It's up to you if you'd like to close this issue or not. I personally do think it's still an issue that will cause bugs in developers projects, but I do see the point brought up previously about not wanting to break existing solutions with an update. I don't know if a comment in the documentation can be considered a solution, but it might be enough.

I was able to find a work around for this so feel free to treat this issue as closed or not depending on how you see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-response A label to track issues where we are waiting for a user response
Projects
None yet
Development

No branches or pull requests

3 participants