-
Notifications
You must be signed in to change notification settings - Fork 97
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
base: master
Are you sure you want to change the base?
Conversation
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.
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.
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, |
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.
This should not be a required argument.
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.
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: |
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.
Magic number
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 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() |
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.
Magic number
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:
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 |
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.
@Janzert So I tried your url and it seems like the second server side filter on |
Correct a typo
Sorry, so besides the obvious date typo it seems like when multiple filters of the same type are applied they get combined as 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. |
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') |
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.
Btw, if you're always going to filter at the level of a full day you can drop the T00:00
.
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.
Right, I just dropped the T00:00
.
Dropped T00:00 in format of requested date as we are filtering the whole day.
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 |
@j-clap can you review? |
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. |
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.