Destination Output and Unit Testing

[davidverweij]

Pulled forked repo from @jawrainey into output branch, to collaborate on this in a separate branch. Continues on PR #14

[davidverweij]

@davidverweij -- I have made the changes you've suggested above and merged master into this PR. Could you test it and let me know your thoughts? Once happy, I can write unit tests for the method (or all methods?!) to put the infrastructure in place for #11, #12 , etc prior to merging this with master. Tests in place will make validating future PR/changes easier -- unless you want to write the tests? 🥇

I will test locally soon and initialize testing infrastructure.

[salmannotkhan]

yup i am done with destination folder too

[salmannotkhan]

i was asking what to do if destination folder is not available

[davidverweij]

@salmannotkhan, could you elaborate on your solution in here please, e.g. code snippets and alike. I haven't seen your code, nor have I tested @jawrainey initial implementation - so I don't have an answer for you yet. Feel free to search and propose a few approaches you think would fix when a destination folder is not available.

[salmannotkhan]

see this is my implementation
docx.write(f"{output}/{file_names[counter] if (custom_name != None) else counter+1}.docx")
i can add verification using os.path library to check whether the directory exists or not
if directory doesn't exist then we can create is using same library

[davidverweij]

I've successfully ran @jawrainey's implementation from his commits

Testing

Default argument

poetry run convert -t tests/data/example.docx -c tests/data/example.csv

Default argument (folder now exists):

poetry run convert -t tests/data/example.docx -c tests/data/example.csv -p output

Nested folders: (folder does not exist):

poetry run convert -t tests/data/example.docx -c tests/data/example.csv -p how/deep/can/we/go/output

Shortcuts (folder does not exist):

poetry run convert -t tests/data/example.docx -c tests/data/example.csv -p ~/Desktop/output

Relative:

poetry run convert -t tests/data/example.docx -c tests/data/example.csv -p ./output

We should NOT merge this until we have written tests to validate the above and whatever other edge cases we can think of -- hence making this a draft. This is my first time using pathlib, so there may be gotchas I am unaware of, but it seems pretty neat.

Originally posted by @jawrainey in #14 (comment)

Note that the option for an output path is -dest, not -p. I will now start to set up unit testing infrastructure

[salmannotkhan]

so is it final?

[salmannotkhan]

is the ~/ shortcuts are working??

[jawrainey]

@salmannotkhan -- the implementation is proposed, but @davidverweij suggested that we start writing unit tests (as per the title) so that we can validate our code and reduce the number of bugs introduced in the future.

~/ is a valid shortcut on UNIX for the home directory.

[salmannotkhan]

so is there any bug in this output destination code?

[salmannotkhan]

I'm not familier with unit test writing so

[jawrainey]

Not that I can find -- can you find any? In short, we can write unit tests to check the above paths automatically, thus validating that our code works. I recommend that you read the following blog post for an overview of testing in python and if you want to learn more there's a good video tutorial on test-driven development.

[davidverweij]

Just to clarify, is this feature now worked on in #20? If so, perhaps I can close this pull request and continue on #20. Issue #23 can separate out the unit-testing enhancement we want to do.

[jawrainey]

Yes, but @salmannotkhan made a new PR in #22 ... so we will close #20 once #22 is complete.

So maybe you can continue on #23 once #22 is resolved?

[davidverweij]

Yes - I meant closing #18, and @salmannotkhan can finish #22.

I will kick off with #23 and rebase once #22 is resolved.