Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added default
sourcefile
behavior togarble.py
andhouseholds.py
#35Added default
sourcefile
behavior togarble.py
andhouseholds.py
#35Changes from 15 commits
8a8364f
8ddf9b0
77735fa
6bdf16a
23ae64b
8b67a68
7c864bf
f529f58
c5c222f
ab5dead
0c8d95d
4afdfb9
f8ef289
9a6b099
268a762
b00dbc8
cf34a72
1573f3f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think this is supposed to be
newest
? The code looks right, just the variable name is backwards. Same thing in households.pyThere 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.
yes and I'm happy to change the name. I chose "oldest" because we're looking for the 'largest' timestamp/furthest from 0 but understand the confusion.
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'm not sure it's worth changing but the YYYYMMDD format means you could just take the maximum. The benefit of parsing I suppose is that you crash if there's anything unexpected in there.
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.
are you suggesting I do
max()
on the timestamps as strings? I see that how/why this would work (and would even work with the current format because the%H%M%S
format gives 24-hr time but would prefer at least parsing the time to make sure it's a time and we're not accidentally comparing an errant file that ended up intemp-data
. A hypothetical"pii-data-report-v2.json"
which would also be picked up by this code and would str-compare to be newer than all of the timestamps because of how python compares strings. That said I could replace a few lines of this logic with amax()
of the parsed timestamp objects, becausedatetime
has comparators built in to it. Does that make sense?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.
If you'd prefer something that's more crash-resistant, I could filter on a regex to ensure the filename is exactly what we're looking for but to me that felt like a little bit overkill, but I'm happy to put it in if you think it's warranted.