-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add --print-path option and README updates #8
Add --print-path option and README updates #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad that you like the tool.
I think this is an improvement, but I think we should improve it some more before merging. You're very welcome to address these comments, but if you don't want to, I'll eventually get around to doing it myself. In the meantime, thank you!
|
||
```sh | ||
apt-get update | ||
apt-get install -y coreutils git python3 python3-tk ghostscript pdftk poppler-utils | ||
git clone https://github.com/svenssonaxel/pdf-sign.git | ||
cd pdf-sign | ||
cp pdf-sign pdf-create-empty /usr/local/bin/ | ||
# Get signatures folder | ||
./pdf-sign "" --print-path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish the empty ""
wasn't needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish too, it's quite ugly ! I'm not an argparse expert but I havn't succeed achieving it using argparse.
Anyway, after reconsideration I'd remove this option, as there is an error message that prints the signatures dir if it doesn't exists. In the README we could write something like:
To know the signature directory to create, run
./pdf-sign my_pdf.pdf
and see the error message
What would you think about this ?
``` | ||
|
||
Now you'll have to put your signature in a PDF file in this folder, you can use `empty-3inx2in.pdf` empty PDF template for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, but the instruction under ## How
also needs correcting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes indeed. I'll merge both sections if you are okay
``` | ||
|
||
Now you'll have to put your signature in a PDF file in this folder, you can use `empty-3inx2in.pdf` empty PDF template for example. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, perhaps add a line such as Thereafter, you can begin using pdf-sign. For example:
Otherwise the below code block could appear like it was about creating signatures.
@@ -6,6 +6,9 @@ import argparse, os, queue, re, subprocess, sys, tempfile, time | |||
|
|||
# Inspired by https://unix.stackexchange.com/a/141496 | |||
def main(args): | |||
if args.print_path: | |||
print(getSignatureDir()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will print the correct path in almost all cases. The exception is when
- The user prefers to put the signatures under
~/.config
in order not to litter the home directory. ~/.config/pdf_signatures
does not exist (yet)$PDF_SIGNATURE_DIR
andXDG_CONFIG_HOME
are both unset.
In this situation, pdf-sign "" --print-path
will print /home/user/.pdf_signatures
even though it should have printed /home/user/.config/pdf_signatures
.
I'm not exactly sure how to fix this, but I think it's a shame to make this improvement without going all the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
But this is out of the scope of this PR, as this is not a problem introduced by this PR, it was already the case before
Anyway, I'll finally suggest to turn back and forget adding this option.
@@ -402,6 +408,7 @@ parser = argparse.ArgumentParser(description='Sign a PDF file.') | |||
parser.add_argument('input', metavar='input.pdf', type=str, help='Input PDF file.') | |||
parser.add_argument('-p', '--page', type=int, default=-1, help='The page to sign, negative for counting from the end. (default: -1)') | |||
parser.add_argument('-s', '--signature', type=str, help='Path to file used as signature. Required in batch mode. In GUI mode, the user can choose among files in $PDF_SIGNATURE_DIR, $XDG_CONFIG_HOME/pdf_signatures (defaulting to ~/.config/pdf_signatures), or ~/.pdf_signatures.') | |||
parser.add_argument('-S', '--print-path', help='Print signatures path.', action='store_true') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps -S
and -s
are confusingly similar?
@brunetton I came up with a different solution that requires no extra options. Essentially, the help text and error messages are customized to explain the situation on your particular system. Check my branch |
@brunetton PR rejected since the solution I mentioned above is now merged. Feel free to open an issue if you are not satisfied. |
Thank you for this great software, I was looking for a tool like this one since some weeks ago.
I used it today and I missed some details to understand how it works. So I updated README and added a command line option to print signatures folder.
Hope this looks okay to you. Tell me !
Thanks again !