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

Added powershell here strings to powershell-restmethod codegen #395

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

karmanya007
Copy link

@karmanya007 karmanya007 commented Oct 24, 2020

Fixes #322 .
Got rid of the escapes by using a PowerShell here-strings in powershell-restmethod codegen.

Request 1

method: 'POST',
    header: [
        {
            key: 'Content-Type',
            value: 'text/plain' 
        }
    ],
    url:{
        raw: 'https://mockbin.org/request',
        protocol: 'https',
        host: [
            'mockbin',
            'org'
        ],
        path: [
            'request'
        ]
    },
    body: {
        'mode': 'raw',
        'raw': 'Hello world'
    },
    description: 'Description'

Conversion

$headers = New-Object "System.Collections.Generic.Dictionary[[String],[String]]"
$headers.Add("Content-Type", "text/plain")

$body = @"Hello world"@

$response = Invoke-RestMethod 'https://mockbin.org/request' -Method 'POST' -Headers $headers -Body $body
$response | ConvertTo-Json

Request 2

method: 'POST',
    header: [],
    body: {
        mode: 'graphql',
        graphql: {
            query: '{ body { graphql } }',
            variables: '{"variable_key": "variable_value"}'
          }
        },
    url: {
        raw: 'http://postman-echo.com/post',
        protocol: 'http',
        host: [
            'postman-echo',
            'com'
        ],
        path: [
            'post'
        ]
    }

Conversion

$headers = New-Object "System.Collections.Generic.Dictionary[[String],[String]]"
$headers.Add("Content-Type", "application/json")

$body = @"{"query":"{ body { graphql } }","variables":{"variable_key":"variable_value"}}"@

$response = Invoke-RestMethod 'http://postman-echo.com/post' -Method 'POST' -Headers $headers -Body $body
$response | ConvertTo-Json

Copy link
Member

@umeshp7 umeshp7 left a comment

Choose a reason for hiding this comment

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

@karmanya007 Can you add a sample output snippet with the new changes?
Also could you add/update unit tests?

codegens/js-xhr/npm-shrinkwrap.json Show resolved Hide resolved
codegens/nodejs-native/npm-shrinkwrap.json Show resolved Hide resolved
test/codegen/newman/fixtures/formdataFileCollection.json Outdated Show resolved Hide resolved
test/codegen/newman/fixtures/formdataFileCollection.json Outdated Show resolved Hide resolved
Copy link
Author

@karmanya007 karmanya007 left a comment

Choose a reason for hiding this comment

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

@umeshp7 Changes added.

@karmanya007
Copy link
Author

@umeshp7 All checks have passed and I have added the sample outputs.

@webholik
Copy link
Collaborator

@karmanya007 I have enabled newman tests for poweshell codegen. These tests run the generated snippet in powershell and compare the output with that generated by newman. You can see in the CI logs that some of the tests are failing due to incorrect escaping of characters in the string. We have to get those tests to pass before we can merge your changes.

@karmanya007
Copy link
Author

@karmanya007 I have enabled newman tests for poweshell codegen. These tests run the generated snippet in powershell and compare the output with that generated by newman. You can see in the CI logs that some of the tests are failing due to incorrect escaping of characters in the string. We have to get those tests to pass before we can merge your changes.

@webholik On it!

@umeshp7
Copy link
Member

umeshp7 commented Nov 3, 2020

@karmanya007 Any updates here? Looks like there are some conflicts.

@karmanya007
Copy link
Author

@umeshp7 Had a marriage in my family so I was not active as much. I know the fix. Just have to implement it. Will do so shortly.

@karmanya007
Copy link
Author

Getting these three errors. @Toronto00 , @umeshp7 Could you guide me in the correct direction on how to approach these conflicts.
1
2
3

@karmanya007
Copy link
Author

karmanya007 commented Nov 15, 2020

@webholik Are the 1st and 3rd error from wrong newman tests, maybe?

@webholik
Copy link
Collaborator

@karmanya007 1st error is because you are not escaping $ character in the output string. If $ is present in a string then Powershell tries to replace it with a variable. In this case it is replacing it with the value of $test, which isn't defined, so it is just putting empty string here.

2nd error again is just wrong escaping. ` character must be properly escaped.

3rd has been fixed in #421. You should rebase your PR on top of it.

@umeshp7 umeshp7 self-assigned this Mar 15, 2022
@akshaydeo akshaydeo assigned webholik and unassigned umeshp7 Feb 15, 2023
@dhwaneetbhatt
Copy link
Contributor

@karmanya007 We recently pushed changes to use here-strings in Powershell. Could you check if this PR would still be required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PowerShell] Simplify generated code by using here-strings
5 participants