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

POST request to an API #111

Merged

Conversation

harshakhmk
Copy link
Contributor

Related Issue

Closes: #https://github.com/IndianOpenSourceFoundation/dynamic-cli/issues/76

Describe the changes you've made

  • Added POST request and implemented various ways to send payload data

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

@harshakhmk
Copy link
Contributor Author

@GouravSardana @pranavbaburaj @NamamiShanker review pls

Copy link
Contributor

@lainq lainq left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to dynamic cli. I have added some review comments. Please let me know your thoughts. 🙂

src/arguments/api_test.py Outdated Show resolved Hide resolved
src/arguments/api_test.py Outdated Show resolved Hide resolved
@GouravSardana
Copy link
Member

Share a GIF or some video to show on various input. So that other can also see how they can use it ?

Copy link
Member

@GouravSardana GouravSardana left a comment

Choose a reason for hiding this comment

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

Please make these small changes and really thankful for your contribution. Great work 🥇

cls.read_data_from_file()
@classmethod
def fetch_payload_data(cls):
store = int(input("Please choose the below options? (1/2/3)"))
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a option here ?
Do you want to send payload Y/N
Then we can display option like send a payload from terminal or file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then there would be multiple conditions

   if yes :
       enter 1 to send from terminal
       enter 2 to read from terminal
       any other number #then again we have to repeat from starting line, if you want to send payload
   elif no 
       return 
   else
          recur the function     

Copy link
Member

Choose a reason for hiding this comment

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

Do we also send bearer token in this payload ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah if the user sends it

@@ -84,6 +116,20 @@ def get_request(cls):
print(cls.invalid_schema_message)
except Exception as exception_obj:
print(exception_obj)
#Make a POST request
Copy link
Member

Choose a reason for hiding this comment

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

Add 2 space above. Follow pep8 formatting please

request_data = cls.fetch_input_url()
data = cls.fetch_payload_data()
try:
response= requests.post(url = request_data["request_url"], headers= request_data["request_headers"], data = data)
Copy link
Member

Choose a reason for hiding this comment

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

Add a space before = (equals)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

everything can be done, using black formatter I guess, shall we try adding it

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you can use the black formatter locally on your system before pushing it 😃

@harshakhmk
Copy link
Contributor Author

Share a GIF or some video to show on various input. So that other can also see how they can use it ?

It's not working on my OS

@GouravSardana
Copy link
Member

How do you test it then ?

@harshakhmk
Copy link
Contributor Author

How do you test it then ?

I tried testing in gitpod, running into errors, new to Linux environment

@GouravSardana
Copy link
Member

No issues. I'll test it before merge @harshakhmk . You can make your changes which are requested

@harshakhmk
Copy link
Contributor Author

No issues. I'll test it before merge @harshakhmk . You can make your changes which are requested

@GouravSardana Thank you for understanding

@harshakhmk
Copy link
Contributor Author

I have made the suggested changes

src/arguments/api_test.py Outdated Show resolved Hide resolved
@harshakhmk
Copy link
Contributor Author

Any other changes?

Copy link
Contributor

@lainq lainq left a comment

Choose a reason for hiding this comment

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

Any other changes?
I think, no. @GouravSardana, please let us know what you think!

@GouravSardana
Copy link
Member

Some Suggestions from my end -

  1. In Screenshot 1 - I could not see 3 options when I clicked on Y.
  2. In screenshot 2 - When I click on 1 then I'll see only 2 options then why did you give 3 options?
  3. In screenshot 3 - When I give a file name. It could not read. It's very painful to add full pathname without having tab access [tab will give autocomplete file in the current folder].
  4. In the last screenshot - When typing in the terminal we can give one more option to open a nano editor and the user can directly copy-paste the JSON which he has. For more info- How to open a nano editor, You can have a subprocess when the user hits option 4 [Option 4 is to open a nano editor to give the input payload] and the User can simply type and exit the nano and It will get the payload from the nano file. You can read this

OnPaste 20210615-161422
OnPaste 20210615-161439
OnPaste 20210615-161459
OnPaste 20210615-161515
OnPaste 20210615-161533

Copy link
Member

@GouravSardana GouravSardana left a comment

Choose a reason for hiding this comment

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

I request the changes above, Please work on it

@GouravSardana
Copy link
Member

For testing any End Point - You can use this - https://reqres.in/

@lainq
Copy link
Contributor

lainq commented Jun 15, 2021

Oh, my bad. Strings don't have an attribute called trim. 🤦🏾‍♂️

if len(filename.strip()) == 0:            
    print("filename empty, so default file (response_data.json) is used ")

I had the wrong method in the example. I am sorry 😐

@harshakhmk
Copy link
Contributor Author

The First 3 issues were resolved, for the time being can we add issue 4 as a new feature in a separate PR @GouravSardana
It can be used in PUT request as well

@GouravSardana
Copy link
Member

Did you push your changes after doing these first 3 issues

@harshakhmk
Copy link
Contributor Author

@GouravSardana yes, I have updated the code

@GouravSardana
Copy link
Member

I'll try it locally once. If everything went well. I'll merge it. Please create an issue for the last screenshot requirement which i have shared above

@harshakhmk harshakhmk requested a review from lainq June 20, 2021 10:52
Copy link
Contributor

@lainq lainq left a comment

Choose a reason for hiding this comment

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

I think it's good. 🙂

src/arguments/api_test.py Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@GouravSardana
Copy link
Member

Everything looks good. Thanks for the contribution @harshakhmk

@GouravSardana GouravSardana merged commit 71004e8 into IndianOpenSourceFoundation:master Jun 21, 2021
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.

3 participants