-
Notifications
You must be signed in to change notification settings - Fork 804
add ppath command #72
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
Conversation
Print application's 'Documents' directory path.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
commands/FBPrintCommands.py
Outdated
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 feel like pdocspath might be a better command here, even if it is a bit verbose. Luckily the prefix matching will then allow pdocs to work as well. Thoughts?
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.
have thought about this before. It seems to me the word docs path is a little ambiguous, maybe docs dir path is better but too long. Here use ppath is just easy to type in. will change to the name suggested.
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.
Thought: have ppath capable of printing other paths other than docs, and have it take an argument to specify which specific path. Otherwise, perhaps ppath can just print the app root, and then it's easy for users to append Documents or any other subpath.
commands/FBPrintCommands.py
Outdated
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.
In other parts of Chisel, in addition to printing a value, we've copied it to the pasteboard. Would you mind adding that here?
|
@dopcn What do you think about having option flags to allow the desired directory to be opened in Terminal, or maybe Finder as well? For example |
not fully tested yet
commands/FBPrintCommands.py
Outdated
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.
Can you make constants for these numbers?
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 could use evaluateObjectExpression() instead of evaluateExpressionValue(), which would make the NSString cast unnecessary.
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.
@kastiglione hi I tried evaluateObjectExpression() and evaluateExpression() but only get pointer value instead of string value so I keep using evaluateExpressionValue() and split the string output in the new implementation. Am I missing something?
|
@dopcn What do you think of the comments I made? |
|
@kastiglione sorry, back and working on it now... |
|
Update copy to the pasteboard before print |
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 could be echo -n to avoid the tr piece.
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 don't know why echo -n just doesn't work as os.system()'s parameter. The -n will be taken as part of pathString and copied into pasteboard
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.
Seems /bin/sh doesn't handle -n like bash and zsh do. Oh well. Thanks for trying.
|
Thanks @dopcn, I'm happy to accept this. Before that, can you merge from |
|
Thanks @dopcn! |
Print application's 'Documents' directory path.