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 upInclude functions for testing existence of derivatives. #10 #37
Conversation
greebie
requested review from
ianmilligan1
and
ruebot
Mar 8, 2019
This comment has been minimized.
This comment has been minimized.
If there is a function that tests the existence of a derivative file, why aren't we displaying that error for the graphs? Just returning two giant empty graphs is going to confuse the user, especially when we do it silently. How about we just return the error that is setout in |
This comment has been minimized.
This comment has been minimized.
I was trying to avoid wrapping the code in an if-else, because it seemed messy. I agree this is not the right approach. Let me try something else, like throw an error or something. |
This comment has been minimized.
This comment has been minimized.
Upcoming commit uses a defined function and |
greebie
added some commits
Mar 11, 2019
ianmilligan1
requested changes
Mar 11, 2019
When running the "user configuration" cell, I received this error:
This was with the default data. |
This comment has been minimized.
This comment has been minimized.
hmm. Cannot replicate this with my repo. Was this the "example" notebook or the plain and do you get the same problem with both? |
This comment has been minimized.
This comment has been minimized.
Also, are you on 3.7 -- fstrings may not work if Python < 3.7. |
This comment has been minimized.
This comment has been minimized.
I can update my Python to see if that matters, but since this worked before, it is possible to use something other than |
This comment has been minimized.
This comment has been minimized.
Yeah, there's a few other DH packages etc. that are still pegged to Python 3.5. Unless its absolutely mission critical, it'd be good to keep it working w/ earlier versions. |
This comment has been minimized.
This comment has been minimized.
I can replace it, but the specs do say we require Python 3.7 or up. I can see the fstrings feature being useful for a project like this. It would be nice to keep it if we can. Fstrings means we can create any Also, it does not look like the error is fstrings-related. It's just that I cannot tell without being able to replicate. |
This comment has been minimized.
This comment has been minimized.
I propose we look at getting this repo to work in 3.7, remove the extra notebook and then do another commit to revert to 3.5 support, even if that means moving to "string".format or something. I can just imagine watching dominos fall trying to accommodate 3.5 in this PR. |
This comment has been minimized.
This comment has been minimized.
Had a change of mind. Switched to .format strings. If we want to support 3.5 we'll need to change the Docker settings. |
This comment has been minimized.
This comment has been minimized.
Closing in favor of #40 . |
greebie commentedMar 8, 2019
This is a response to #10 that includes the following:
thing if TEXT else []
syntax to produce empty graphs instead of an error when no derivative exists.To test:
I've been changing the filenames from
4867-fullurls.txt
toNOT4867-fullurls.txt
and then running the windows to see how it works. Then you just remove the NOT to restore the original.