-
Notifications
You must be signed in to change notification settings - Fork 77
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
Write to file without including "filename" column #317
Conversation
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
df, | ||
output_file_dir, | ||
write_to_filename=False, | ||
keep_filename_column=False, |
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.
Interested in others' opinions here. I think it makes the most sense to default to false here, and automatically drop the "filename" column unless specified otherwise. It saves storage space and the column itself means nothing to the user who can already see the filename after it is written.
I know a lot of our classifier scripts, etc. use the write_to_disk
, so those will all now automatically default to keep_filename_column=False
. Don't know if we should consider adding it to argparse
for more user control on the CLI side, or if folks think we should be good without.
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, defaulting to False
is good. I think it was an oversight when initially making this. I wouldn't add as an option to argparse yet.
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.
Agreed, this behavior makes a lot more 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.
Looks good to me, only ask is to add type hints.
Thanks for fixing this for us.
* keep_filename_column param Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> * update pytests Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> * run black Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> --------- Signed-off-by: Sarah Yurick <sarahyurick@gmail.com>
* keep_filename_column param Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> * update pytests Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> * run black Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> --------- Signed-off-by: Sarah Yurick <sarahyurick@gmail.com> Signed-off-by: Ayush Dattagupta <ayushdg95@gmail.com>
Closes #293.