Three column page#27
Conversation
|
Hello @Will-Shanks! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-03-11 22:30:34 UTC |
JadeWibbels
left a comment
JadeWibbels
left a comment
There was a problem hiding this comment.
There has been some changes to master. You will want to merge master back into branch asap to keep new code up to date.
|
When this code was moved from the task2 branch to its own, the new branch was created off master. This means you don't need to worry about merging master into this branch, you just need to use the functionality provided by the code base already, namely the 2 functions provided by DayToDF.py (get_df and iter_df) |
|
I had a go at simplifying the code, I also shortened the filename to three_col.py. |
| # iter through all lines | ||
| for line in dayToDF.iter_df(dayToDF.get_df(fn)): | ||
| # If line contains a key word, then is a company name | ||
| if line.split(' ', 1)[0].isupper() and ("Inco" in line or "In-" in line |
There was a problem hiding this comment.
In- may not be working since dayToDF uses non-punctuation words
There was a problem hiding this comment.
Good point, I'll have a look at what the best way to fix this is.
qisu4484
left a comment
qisu4484
left a comment
There was a problem hiding this comment.
May not get all desired output, missing keywords
@lululu7 |
For the 2/3 col pages, the common words after company names are "Incorporated", "Organizes", and "Controlled" |
|
It looks like this should be ready to be approved. I think the only outstanding issue right now is Jades change request regarding merging to master. Once we resolve this change request I think we can merge. |
Jade's comments were already handled (see earlier comments), but Lulu found a couple potential bugs that need looking into before this gets merged. |
I saw that also :) I think the request is still open on github though preventing the merge. For record keeping purposes I think Lulu should make a change request for the bugs and we should close jades about merging to master. Thoughts? (I might just be confused :) ) |
|
I think what happened it Lulu made change requests and then approved the pr as well, which I think made her change required obsolete. I was going to try fixing it all up and then get both Jade and Lulu to look over it again. |
8 participants
No description provided.