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 date as an argument in the replay.user mode in the hlt client #426

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

iamtomcheng
Copy link

This is a quick fix by adding the date as argument in the respective methods and classes and adding an if statement to skip replays that belong to the wrong date in case the requested date is given as input. Please feel free to amend it to make it more efficient.

Add '-t' and '--date' as the argument for Modes.Replay.Modes.User
Correct a mistake
Add date as the argument for UserGameDownloader and skip replays that belongs to the wrong date in the method _fetch_metadata in case date is given as the input.
Copy link
Contributor

@j-clap j-clap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a clean logic for date download. Also, magic vars all around. Please address those.

@@ -184,6 +184,9 @@ def _parse_arguments():
help='Number of replays to fetch')
replay_user_parser.add_argument('-d', '--destination', dest='destination', action='store', type=str, required=True,
help="In which folder to store all resulting replay files.")
replay_user_parser.add_argument('-t', '--date', action='store', type=str, dest='date', required=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a required argument.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will change it to 'default=None' and remove 'required=True'.

result_set += requests.get(self._USER_BOT_URI.format(user_id, current_limit, current)).json()
current += self._FETCH_THRESHOLD
else:
if requests.get(self._USER_BOT_URI.format(user_id, 1, current)).json()[0]["replay"].split("-")[1] != date:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to check the date of the retrieved replay one at a time. Do you have any suggestion on how to do this faster? I can also retrieve current_limit replays at a time and filter out replays that belongs to the wrong date and append the filtered list to the list result_set.

limit += 1
continue
else:
result_set += requests.get(self._USER_BOT_URI.format(user_id, 1, current)).json()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number

@iamtomcheng iamtomcheng changed the title Add date as argument user mode of the replay mode in the hlt client Add date as an argument in the user mode of the replay mode in the hlt client Jan 10, 2018
@iamtomcheng iamtomcheng changed the title Add date as an argument in the user mode of the replay mode in the hlt client Add date as an argument in the replay.user mode in the hlt client Jan 10, 2018
@Janzert
Copy link
Contributor

Janzert commented Jan 10, 2018

You probably don't want to be filtering on the date client side, rather use the server side filtering. For example to get the games for iamtomcheng on new years day use a url something like this:

https://api.halite.io/v1/api/user/4882/match?limit=250&filter=time_played,>=,2018-01-01T00:00&filter=time_played,<,2017-01-02T00:00

You'll still generally need to make multiple requests with appropriate limit and offsets to get the complete day.

Also if you do want to do any date filtering on the client side it'd probably be best to use the time_played field returned with the metadata rather than parsing it out of the replay name.

Filter replays from the server side when the date is given in the method _fetch_metadata of the class UserGameDownloader.
Retrieve replays starting on the requested date on server side and filter out the retrieved replays that were played on the next day and onwards in method _fetch_metadata of class UserGameDownloader.
@iamtomcheng
Copy link
Author

iamtomcheng commented Jan 12, 2018

@Janzert So I tried your url and it seems like the second server side filter on time_played is not working, i.e. 250 replays starting from 2018-01-01T00:00 are returned, and also you put 2017 instead of 2018. I removed the second filter on time_played, add the offset back and add a client side filter using the time_played field to keep the logic simple.

Correct a typo
@Janzert
Copy link
Contributor

Janzert commented Jan 12, 2018

Sorry, so besides the obvious date typo it seems like when multiple filters of the same type are applied they get combined as first_filter OR second_filter. So the above query ends up matching for every game.

Filters of different types (e.g. game_id and time_played) do get applied with AND, so I have no idea what is going on.

And yes no matter what any single query will only return a maximum of 250 games, that's what I meant about needing to use multiple requests with appropriate limit and offsets.

@iamtomcheng
Copy link
Author

Hi @j-clap, could you take a look at the modifications and see if there is still any other issue? Thanks.

if date is None:
result_set += requests.get(self._USER_BOT_URI.format(user_id, current_limit, current)).json()
else:
requested_date = datetime.datetime.strptime(date,'%Y%m%d').strftime('%Y-%m-%dT00:00')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, if you're always going to filter at the level of a full day you can drop the T00:00.

Copy link
Author

@iamtomcheng iamtomcheng Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I just dropped the T00:00.

Dropped T00:00 in format of requested date as we are filtering the whole day.
@iamtomcheng
Copy link
Author

Hi @harikmenon, could you take a look at the changes made and see if it is okay now. Also, as pointed out by @Janzert, there is a bug (?) in the Halite API that when multiple filters of the same type are applied they get combined as first_filter OR second_filter instead of first_filter AND second_filter. Maybe you would like to fix that as well if it was not intentional.

@harikmenon
Copy link
Contributor

@j-clap can you review?

@Janzert
Copy link
Contributor

Janzert commented Jan 17, 2018

To be clear the OR'ing of multiple uses of a filter in the api is definitely intentional. I'm not sure when the capability is used, but it is special cased in the code to make it OR instead of AND. Certainly both are useful in different situations.

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

Successfully merging this pull request may close these issues.

4 participants