Add destination folder #14
Conversation
Used type annotation and docstrings for new method.
csv2docx/cli.py
Outdated
| import pathlib | ||
|
|
||
|
|
||
| def create_path_if_not_exists(path: str) -> pathlib.Path: |
There was a problem hiding this comment.
I don't like this method name ... open to suggestions. It's nice to keep it separate though as we write separate tests for it. IMHO, do not merge these changes until tests exist 👍
There was a problem hiding this comment.
Also, the problem is that the parameter path can be a directory output, an absolute path /user/bin/output or relative path ../../output and we should accept all those conditions.
Rather than deal with edge cases, I used pathlib as it converts the given path to a concrete path that is user platform specific 👍
There was a problem hiding this comment.
Perhaps go with
def concrete_path(path: str) -> pathlib.Path:
with a note that this method will create the directory if non-existent.
Ps. does it handle a nested non-existent directory?
There was a problem hiding this comment.
Great suggestion re:name 👍
Yes -- it handles nested non-existent directories thanks to the parents option.
There was a problem hiding this comment.
How about absolute_path as it's more common terminology?
csv2docx/cli.py
Outdated
| print ("Getting .docx template and .csv data files ...") | ||
| help='Delimiter used in your csv. Default is \';\'') | ||
| @click.option( | ||
| '--path', '-p', |
There was a problem hiding this comment.
Maybe destination is a better name?
There was a problem hiding this comment.
I would go with destination yes
There was a problem hiding this comment.
@davidverweij -- one minor issue with destination is that the data option has the -d flag ...
There was a problem hiding this comment.
i would go with -o output it feels standard
TestingDefault argument Default argument (folder now exists): Nested folders: (folder does not exist): Shortcuts (folder does not exist): Relative: 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. |
csv2docx/cli.py
Outdated
| return | ||
|
|
||
| print("All fields are present in your csv. Generating Word docs ...") | ||
|
|
||
| path = create_path_if_not_exists(path) |
There was a problem hiding this comment.
As above, but to reiterate: can you think of a better method name?
csv2docx/cli.py
Outdated
| docx.merge_templates([single_document], separator='page_break') | ||
| # TODO: write to user-defined subfolder | ||
| docx.write(f"{counter}.docx") | ||
| docx.write(f"{path}/{counter}.docx") |
There was a problem hiding this comment.
Just had a thought that this might cause issues on windows given we are using /. However, because we convert path to type pathlib.Path it means that once we reach this docx.write that it is now a pathlib.path and not a string. This is very important because if you use the backslash operator (/) after a pathlib.path object then it will join whatever is right (in this case, the filename) with the path 👍
There was a problem hiding this comment.
So that means the "/" will work in this case right? Also, the counter would ideally be a filename based on the .csv - see also #16 - but that might be for another PR
There was a problem hiding this comment.
Yes, it would work in this case, but I feel that we should probably instead create a variable before adding it to docx.write to make it more explicit, e.g. write(f"{path}.docx") where path is defined before 👍
|
@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? 🥇 |
|
Pulled into 'output_destination' branch to work on further. Please |
|
Continuing this on the output branch, see PR #18 |
No description provided.