Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upCopyediting #22
Conversation
ruebot
added some commits
Mar 5, 2019
ruebot
reviewed
Mar 5, 2019
" :param by: \"all\", \"domain\" or \"year\" the output to return\n", | ||
" :param minline: the minimum size of a line to be included in the output\n", | ||
" :param by: \"all\", \"domain\" or \"year\" the output to return.\n", | ||
" :param minline: the minimum size of a line to be included in the output.\n", | ||
" :return: [(year/domain, text)] or [text] depending on by\n", |
This comment has been minimized.
This comment has been minimized.
ruebot
Mar 5, 2019
Author
Member
@greebie the return comment here seems to be missing something. What should it read?
This comment has been minimized.
This comment has been minimized.
greebie
Mar 5, 2019
Collaborator
Maybe :return: [({year or domain}, textString)] if
by is 'domain' or 'year', otherwise [textString]\n
is more clear - it is also possible to include the type using :rtype: but I don't think that is too necessary.
greebie
approved these changes
Mar 5, 2019
ianmilligan1
reviewed
Mar 5, 2019
"# characters to show per text file in output. Larger numbers will results in more\n", | ||
"# text showing in output\n", | ||
"# Characters to show per text file in output.\n", | ||
"# Larger numbers will results in more text showing in output.\n", |
This comment has been minimized.
This comment has been minimized.
greebie
reviewed
Mar 5, 2019
"# If you have a large file but want to sample the file more broadly, you\n", | ||
"# can increase this value skip to every Nth line.\n", | ||
"# If you have a large file but want to sample the file more broadly.\n", | ||
"# You can increase this value skip to every Nth line.\n", | ||
"RESULTS_STEP = 5\n", |
This comment has been minimized.
This comment has been minimized.
greebie
Mar 5, 2019
Collaborator
I wonder if RESULTS_STEP should be changed to 1
for the default example? This var (plus RESULTS_START & RESULTS_LIMIT) is used basically so that we do not potato browsers. Right now, the analysis takes every 5th line starting from 0 and going to 2500, for a total of 500 lines. Alternately, we could keep RESULTS_STEP the same, and increase RESULTS_LIMIT to get more results.
greebie
reviewed
Mar 5, 2019
"def write_output (stdout, results):\n", | ||
" \"\"\" Writes results to file.\n", | ||
" \n", | ||
" :param stdout: Filepath for file\n", | ||
" :param results: A list of results.\n", | ||
" :return: Nothing\"\"\"\n", | ||
" :return: Nothing\n", |
This comment has been minimized.
This comment has been minimized.
greebie
reviewed
Mar 5, 2019
"mapping = {x[0]: x[1] for x in graph.nodes('label')}\n", | ||
"\n", | ||
"# Use Archive Unleashed Clouds Positions (saves on load time)\n", | ||
"# Use Archive Unleashed Clouds Positions (saves on load time).\n", |
This comment has been minimized.
This comment has been minimized.
greebie
reviewed
Mar 5, 2019
"mapping = {x[0]: x[1] for x in neigh.nodes('label')}\n", | ||
"\n", | ||
"# Use Archive Unleashed Clouds Positions (saves on load time)\n", | ||
"# Use Archive Unleashed Clouds Positions (saves on load time).\n", |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
If we're good with the review commit, I'll move on to the other notebook. Then get the other dataset in other PR. |
greebie
approved these changes
Mar 5, 2019
This comment has been minimized.
This comment has been minimized.
changes lgtm. |
ruebot
marked this pull request as ready for review
Mar 5, 2019
This comment has been minimized.
This comment has been minimized.
Both notebooks are added now. Let me know if there are any issues. If there are none, commit message should just be: Title: |
ruebot commentedMar 5, 2019
•
edited
Close read of text and comments. I'll start with the example notebook, and move on to the other.