Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upReworks file outputs, resolves #44 #56
+409
−392
Conversation
ianmilligan1
requested review from
greebie,
ruebot and
SamFritz
Apr 25, 2019
greebie
reviewed
Apr 25, 2019
| "# Change if you want a different filename.\n", | ||
| "\n", | ||
| "OUTPUT_FILENAME = \"./filtered_text.txt\", # filename if you want to output to another file.\n", | ||
| "\n", |
This comment has been minimized.
This comment has been minimized.
greebie
Apr 25, 2019
Collaborator
Just a note here that removing this element does not mean that OUTPUT_FILENAME does not have a value, since this variable (actually, au_notebook.output_filename) will have a value in the class that can be used.
This comment has been minimized.
This comment has been minimized.
ianmilligan1
Apr 25, 2019
Author
Member
Do you think this is OK to remove then - what do you recommend?
This comment has been minimized.
This comment has been minimized.
greebie
Apr 25, 2019
•
Collaborator
Yes. Fine to remove for this. There may be a consideration for the au_notebook class, as it appears that write_output() does not even use it as a default value. If OUTPUT_FILENAME is not set, nb.output_filename is "./filtered_text.txt". I say it's fine to be hidden in the shadows for now.
This comment has been minimized.
This comment has been minimized.
SamFritz
approved these changes
Apr 26, 2019
ruebot
merged commit ab76abe
into
master
Apr 28, 2019
1 check passed
ci/dockercloud
Your tests passed in Docker Cloud
Details
ruebot
deleted the
issue-44
branch
Apr 28, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ianmilligan1 commentedApr 25, 2019
Rationale for Pull Request
#44 noted that some of our datathon attendees were finding the commenting out a bit difficult to follow – on review, this makes sense, as it's the only part of the notebook that requires the removal of a comment. I don't think outputting a text file is so catastrophic that we need to need a user to manually uncomment out a line, and hopefully they're at least reading the language around what they are executing.
And with #54 merged, we are all good to go on that front (thanks @greebie!).
What this Changes
This PR makes two main changes:
nb.write_outputcells.How Should it be Tested
@greebie should make sure it also works on his end, and maybe we should quickly chat that you're ok with removing the global variable for file outputs. I think we should try to keep it as a simple as possible and this method removes the variable futzing.
Example Screenshot