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

Accept file as payload with @ token #181

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Ishmeet
Copy link

@Ishmeet Ishmeet commented May 10, 2024

#173

ishmeets@ishmeets-mbp protocurl % ./src/bin/protocurl -I test/proto -i ..HappyDayRequest ..HappyDayResponse -u localhost:8080/happy-day/verify -d @file.txt                
=========================== POST Request  Text    =========================== >>>
includeReason: true
=========================== POST Response Text    =========================== <<<
1: 1
2: "Thursday is a Happy Day! ⭐"
3: "Thu, 01 Jan 1970 00:00:00 GMT"
4: ""

Copy link
Collaborator

@GollyTicker GollyTicker left a comment

Choose a reason for hiding this comment

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

Overall it looks good. Thanks for working on it.

Can you also add tests please? Take a look at TESTS.md and you should be able to run and configure them and add new ones.

src/flags.go Outdated Show resolved Hide resolved
src/flags.go Outdated Show resolved Hide resolved
src/flags.go Show resolved Hide resolved
@Ishmeet
Copy link
Author

Ishmeet commented May 10, 2024

Overall it looks good. Thanks for working on it.

Can you also add tests please? Take a look at TESTS.md and you should be able to run and configure them and add new ones.

hmm.. getting this error

ishmeets@ishmeets-mbp protocurl % ./test/suite/test.sh "$PWD"
./release/0-get-latest-dependencies-versions.sh: line 70: conditional binary operator expected

@GollyTicker
Copy link
Collaborator

GollyTicker commented May 10, 2024

Overall it looks good. Thanks for working on it.
Can you also add tests please? Take a look at TESTS.md and you should be able to run and configure them and add new ones.

hmm.. getting this error

ishmeets@ishmeets-mbp protocurl % ./test/suite/test.sh "$PWD"
./release/0-get-latest-dependencies-versions.sh: line 70: conditional binary operator expected

Which OS, shell, etc. are you using?

Edit: A sufficiently modern bash should actually do it. The [[ ! -v LATEST_VERSION ]] should actually work quite reliably... It simply checks, if the variable is defined without causing a problem given the -u shelloption which is set.

@GollyTicker
Copy link
Collaborator

Ah, I forgot to mention this section from the main README.md
https://github.com/qaware/protocurl/?tab=readme-ov-file#development

@Ishmeet
Copy link
Author

Ishmeet commented May 10, 2024

Overall it looks good. Thanks for working on it.
Can you also add tests please? Take a look at TESTS.md and you should be able to run and configure them and add new ones.

hmm.. getting this error

ishmeets@ishmeets-mbp protocurl % ./test/suite/test.sh "$PWD"
./release/0-get-latest-dependencies-versions.sh: line 70: conditional binary operator expected

Which OS, shell, etc. are you using?

Edit: A sufficiently modern bash should actually do it. The [[ ! -v LATEST_VERSION ]] should actually work quite reliably... It simply checks, if the variable is defined without causing a problem given the -u shelloption which is set.

ishmeets@ishmeets-mbp protocurl % sw_vers    
ProductName:		macOS
ProductVersion:		13.6.2
BuildVersion:		22G320
ishmeets@ishmeets-mbp protocurl % echo $SHELL
/bin/zsh

Tried with bash also, same error.

@GollyTicker
Copy link
Collaborator

You might want to debug the shebang and the spcific zsh/bash version:
https://superuser.com/questions/1616706/v-unary-conditional-expression-isnt-working

I'm not sure, but please check which bash version you're specifically using. My Bash 5.1 definitely does work.

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

Successfully merging this pull request may close these issues.

None yet

2 participants