Skip to content
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

Reworks file outputs, resolves #44 #56

Merged
merged 1 commit into from Apr 28, 2019

Conversation

Projects
None yet
4 participants
@ianmilligan1
Copy link
Member

commented Apr 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:

  • First, it removes the global variable for filenames because there are two different places we have outputs: years and domains. I think since they're writing files, and might be writing multiple files with different years, domains, etc., it doesn't make sense to use a variable and instead they should just provide the name of the file they're outputting to. Right now, you could easily imagine a user having to consistently hop between the executing cell and the set up cell.
  • Second, it adds markdown explanations and new nb.write_output cells.

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

Screen Shot 2019-04-25 at 8 17 28 AM

@ianmilligan1 ianmilligan1 requested review from greebie, ruebot and SamFritz 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.

Copy link
@greebie

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.

Copy link
@ianmilligan1

ianmilligan1 Apr 25, 2019

Author Member

Do you think this is OK to remove then - what do you recommend?

This comment has been minimized.

Copy link
@greebie

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.

Copy link
@ianmilligan1

ianmilligan1 Apr 25, 2019

Author Member

OK sounds good, thanks @greebie !

@greebie
Copy link
Collaborator

left a comment

lgtm

@ruebot ruebot merged commit ab76abe into master Apr 28, 2019

1 check passed

ci/dockercloud Your tests passed in Docker Cloud
Details

@ruebot 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
You can’t perform that action at this time.