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

Add to parameter to statistics API #202

Closed

Conversation

martarho
Copy link

@martarho martarho commented Nov 25, 2022

PR to add:

  • Support for to parameter on the statistics API
  • Adjust function documentation
  • Adjust daily statistics requests when from_date == today to match the general closed-interval behavior of the API
  • Adjust default (when from and/or to are set to None) behavior of non-day statistics to match calendar intervals

Some notes that might be of interest to document somewhere:

  • The API generally works with closed-intervals, meaning it includes the from date and to date in the results EXCEPT if you set the from date to today. To match the general behavior of the API, I adjusted the code so when from_date == today, it returns today's trips. I can't test at the moment if this could also solve the problem with returning values at the beginning of weeks/months, but I can test further in the following weeks.
  • When requesting any non-day intervals, the from/to dates play a big role. If the user sets them to arbitrary dates (meaning: either or both dates don't match the beginning and end of the interval used) the API returns a summary of the trips within those two dates as if they represented a whole interval (i.e. a given month). The way data is presented can be confusing as it seems like it shows the data from the full calendar interval, not the requested interval. --> Open Question Should we enrich the result data with the requested date intervals so the data is more interpretable?
  • The client default behavior for non-day intervals is set to match the calendar intervals
    • Week: Sun to Sat interval of current week
    • Isoweek: Mon to Sun interval of current week
    • Month: 1st day to last day of the current month
    • Year: 1st January to 31st December of the current year
  • Generally, the API seems more flexible with the to parameter than with the from and it can be set to a future date without making the API crash.

@martarho
Copy link
Author

martarho commented Nov 25, 2022

@DurgNomis-drol Hi there, here you have the PR! I can't link the related issue here, though, due to lacking permissions.

Let me know if there's any questions/comments/feedback before merging :)

@DurgNomis-drol
Copy link
Owner

Thanks for your contribution 💯

Please fix the cli, you can run pre-commit before you commit to test locally.

We also need at second person to test if it works before this will be merge into master. But so far it looks good 🚀

@CM000n
Copy link
Collaborator

CM000n commented Nov 21, 2023

I think we can close this PR since this is now more than 1 year old and the requirements where never fullfilled.

@martarho
Copy link
Author

Hi, sorry, i didnt get any notification from this. Give me a few days to do the changes and update the code

@CM000n
Copy link
Collaborator

CM000n commented Nov 24, 2023

Hello @martarho, nice that you got the message. 😊
I don't think you need to revise your MR any more?
Work is already underway on a major switch to a new API from Toyota. Perhaps you would like to contribute your input there: #242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants