-
Notifications
You must be signed in to change notification settings - Fork 82
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
Should we have a new validate()
function?
#48
Comments
I don't think this is a good idea, on further consideration. It goes against the try-except flow we're using. The CLI output at the moment is superfluous, I agree, but I don't think this is the right approach as it will end up duplicating code. I think a better approach would be to tell the user what folder is being used once the file list is found. I'll play with this a bit and get back to you. |
Please review the code over in #58 and see what you think of the output. |
You're right. I like the idea of being consistent. I just tested #58 as you asked, using an invalid path, and it acted odd to me: first it complained about the directory not found, and then it processed a file, then it complained again, and processed the other file. I understand, technically, that it just does a fallback to the default which is where 2 files happen to be, but that doesn't rhyme with the error messages then. Thoughts? |
It's 2 different sections that are being used. So it makes sense to try the
path each time.
…On 25 Oct 2017 21:48, "Torben Gundtofte-Bruun" ***@***.***> wrote:
goes against the try-except flow we're using
You're right. I like the idea of being consistent. I just tested #58
<#58> as you asked, using an
invalid path, and it acted odd to me: first it complained about the
directory not found, and then it processed a file, then it complained
again, and processed the other file. I understand, technically, that it
just does a fallback to the default which is where 2 files happen to be,
but that doesn't rhyme with the error messages then. Thoughts?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#48 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/APeoH6MwIXDvqh713Dov4Yk86IHntiDgks5sv56ugaJpZM4QCcm_>
.
|
Do you feel this looks like what you expected:
|
Yeah, that's what I would've expected. Maybe it would be better to specify the format first, but that makes sense to me. |
I'm sorry but that output doesn't make sense to me as a user...
I hope I didn't offend, it isn't meant to! |
Ok, the first point makes sense. I think the second point would be cleared
up by stating the format beforehand also. I think it's necessary to state
it for each section in case of custom directories being set in the
individual formats.
…On 25 October 2017 at 22:36, Torben Gundtofte-Bruun < ***@***.***> wrote:
I'm sorry but that output doesn't make sense to me as a user...
- "First it says it can't find the download, and then it still
processes something? How?" Maybe it should then also state what directory
it defaults to. That would clear up some confusion.
- "It has already told me once that it couldn't find my directory;
small wonder that it still can't find it half a second later, eh?" I
understand that it happens because of where it is in the code, but it
doesn't look too smart.
I hope I didn't offend, it isn't meant to!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#48 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/APeoH_6LCkvl0fiZur84GtZQ3fTlI4G0ks5sv6nwgaJpZM4QCcm_>
.
|
That's a valid point! |
Changed things around a bit. Example output below.
|
This should be resolved appropriately in master now. |
As originally suggested here perhaps
main()
should call a newvalidate()
function where a variety of validation checks could live?The text was updated successfully, but these errors were encountered: