Skip to content
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

pcurl command #109

Merged
merged 15 commits into from Sep 28, 2015
Merged

pcurl command #109

merged 15 commits into from Sep 28, 2015

Conversation

VTopoliuk
Copy link

Added ability to convert NSURLReuqest to curl command.
Example:

  NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:[NSURL URLWithString:@"https://google.com"]];
  request.timeoutInterval = 30;
  request.HTTPMethod = @"POST";
  request.allHTTPHeaderFields = @{@"Header1" : @"test-1", @"Header2" : @"some test sttring"};
  request.HTTPBody = [@"sample data" dataUsingEncoding:NSUTF8StringEncoding];
curl -X POST --connect-timeout 30 -H "Header2: some test sttring" -H "Header1: test-1" --data-binary @"/tmp/curl_data_463408775.11118102" "https://google.com"

Also you can embed data into terminal command with --embed-data parameter, result:

echo "c2FtcGxlIGRhdGE=" | base64 -D -o "/tmp/curl_data_463487823.35579801"; curl -X POST --connect-timeout 30 -H "Header2: some test sttring" -H "Header1: test-1" --data-binary @"/tmp/curl_data_463487823.35579801" "https://google.com"

Dave Lee and others added 2 commits June 2, 2015 09:59
Added ability to convert NSURLReuqest to curl command.
def run(self, arguments, options):
request = arguments[0]
HTTPHeaderSring = ''
HTTPMethod = fb.evaluateExpressionValue('(id)[{request} HTTPMethod]'.format(request=request)).GetObjectDescription()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer:

HTTPMethod = fb.evaluateExpressionValue("(id)[%s HTTPMethod]" % (request)).GetObjectDescription()

But then again, I ain't no Python guru.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IANAPG either, but I believe format is preferred over %. However, format does not require named args, so a middle ground is:

HTTPMethod = fb.evaluateExpressionValue('(id)[{} HTTPMethod]'.format(request)).GetObjectDescription()

EDIT: However there's a readability advantage to the way @VTopoliuk has it now.

@mattjgalloway
Copy link
Member

Overall LGTM, sans the comments. @kastiglione - any thoughts before we merge?

@arigrant
Copy link
Contributor

arigrant commented Sep 8, 2015

Looks great to me! I'll let @kastiglione take a peek as well though.

@kastiglione
Copy link
Contributor

Great addition, thanks @VTopoliuk!

I think this just needs to error when run on device and is good to go.

@VTopoliuk
Copy link
Author

I added --portable parameter to embed data into the final command. So now iOS Device is supported, but script will fire error if executed without --portable on device and request has body data.


def options(self):
return [
fb.FBCommandArgument(short='-p', long='--portable', arg='portable', boolean=True, default=False, help='Embed request data as base64.'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be useBase64Data or something? portable seems like a strange name for this parameter.

Or, just always do base64 data?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattjgalloway actually 'portable' in this case means portable command or "you can run this command on other computers, not current one only"
http://dictionary.reference.com/browse/portable

base64 looks ugly and contains overhead on every run, not sure always output it is a good option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get how 'portable' makes sense here still. It sort of makes sense, but not intuitive to me.

Since you're copy-pasting the curl command anyway, I don't see why it matters that the base64 is ugly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my real-life test base64 is longer than one screen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. Big amounts of data!

I still do think that portable is a bad name. Maybe even it should be dataStyle and then options are tmp or base64 and it defaults to the one which makes most sense for the current platform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to see what @kastiglione / @arigrant think too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"portable" is definitely opaque to me. I'm good with --base64. Other suggestions: --inline(-data), --embed(-data)

@VTopoliuk
Copy link
Author

I updated parameter name

@mattjgalloway
Copy link
Member

@VTopoliuk How about the nit? Can you fix that and then I think we're good to go!

if len(HTTPHeaderSring) > 0:
commandString += ' ' + HTTPHeaderSring
if dataFile is not None:
commandString += ' --data-binary @"{}"'.format(dataFile)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattjgalloway actually same file are used in two cases:

  1. To embed data
  2. To write data to file

If I will place name generation under the not runtimeHelpers.isIOSDevice(): condition, then it will be not available here with --embed-data option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops sorry!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we go?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kastiglione do you think it's ready for merge?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple comments. Primarily needs a fix for the location of dataFile.

if fb.evaluateIntegerExpression('[{} respondsToSelector:@selector(base64EncodedStringWithOptions:)]'.format(HTTPData)):
dataAsString = fb.evaluateExpressionValue('(id)[(id){} base64EncodedStringWithOptions:0]'.format(HTTPData)).GetObjectDescription()
elif not runtimeHelpers.isIOSDevice():
fb.evaluateExpression('(BOOL)[{} writeToFile:@"{}" atomically:NO]'.format(HTTPData, dataFile))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to at least warn if this fails.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are totally right!

mattjgalloway and others added 2 commits September 15, 2015 08:23
Prevent overly eager regex match in `fv` command
@kastiglione
Copy link
Contributor

Looks like you'll need to merge master into this branch now.

Vitalii Topoliuk added 2 commits September 16, 2015 11:38
@kastiglione
Copy link
Contributor

@VTopoliuk shall we merge this? cc @mattjgalloway

@mattjgalloway
Copy link
Member

Let's do it.

kastiglione added a commit that referenced this pull request Sep 28, 2015
@kastiglione kastiglione merged commit 033def7 into facebook:master Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants