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
#13 Spreadsheet download #22
#13 Spreadsheet download #22
Changes from 38 commits
afc8e27
578b89e
357cd6c
f09205d
f606b50
eeee548
5072a1c
db17cce
6fd08d7
fb4938b
a16c966
37aa895
29e1834
74b103b
8168a9f
9259ef0
99470a4
c0efbac
2399420
7d0b653
a10cf5b
bb7b2ce
34c3271
456a034
72fbb24
d000bc5
600a913
b567a7c
6e82048
ba9bc9a
03b5336
7f3ba1a
8fb0560
63ecd2b
321c5b8
2b9bcd1
7abbdcf
6269a3d
0807fec
78e732d
e757244
dd56ad4
e782e6d
099e5fc
781007f
da14c58
9342eb6
2ccc5bd
96cf299
0b4d5a8
0c89235
b0fba31
09d7a8f
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.
Should there be an
else
path as well that raises an exception to protect client from unsupported use?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.
There's no need for that, if you check where this function is being called, it can only be called if it's a dict or list.
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.
Could it be called by someone else using different arguments?
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.
Why are some of these methods returning values and some modifying properties on the object without returning it? In general, I think it's bad practice for a function to modify an object since it can cause unexpected side effects. I think it would be better to copy the object, change it, and return it
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.
These are private functions so it shouldn't be a big concern. They are needed to be modified because it's part of the recursive logic. I'm not yet sure how to achieve it with your suggestion to copy the object, change it, and return it. I will think about it. Could we leave that for later?
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.
Yeah no problem. I was just curious more than anything. Being private makes 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.
I agree with @jacobwindsor that pure functions are an ideal to aspire to, since side0effects can be a source of surprise and therefore bugs. Sometimes, if we make an explicit decision to have side-effects, the function's name should make it clear.