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

feat: add curl config #115

Merged
merged 5 commits into from
Jun 13, 2024
Merged

feat: add curl config #115

merged 5 commits into from
Jun 13, 2024

Conversation

zhangymPerson
Copy link
Contributor

No description provided.

Copy link
Collaborator

@smeech smeech left a comment

Choose a reason for hiding this comment

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

A straightforward text replacement package.
Well documented, and tags: in _manifest.yml.
Looks safe to merge, but does present various curl commands, so needs double-checking/approval by someone more experienced with these than me.

@smeech
Copy link
Collaborator

smeech commented Jun 11, 2024

I've done some reading around curl and am happy the package is pretty safe, even if curl itself has risks. I'll merge it subject to one issue - see Review, below.

It might be interesting to add a global variable, along the lines of:

global_vars:
  - name: curlform
    type: form
    params:
      layout: Target https://[[URL]]

and replace all instances of example.com with {{curlform.URL}} e.g.:

matches:
  - trigger: ":curl:"
    replace: "curl --request GET https://{{curlform.URL}}"

The other optional variables: value, key, file/path etc. could also be supplied by forms in a similar manner, or cursor hints used to position the cursor in simpler cases.

replace: "curl --request PATCH https://example.com"

- trigger: ":curldata:"
replace: "curl --request POST --header 'Content-Type: multipart/form-data' --form 'key=value' http://example.com"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for the mix of https: and http:? The former throughout might be safer? I'd be happy to merge if this is addressed (unless there are good reasons not to).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added the relevant global variables as per your prompt

@zhangymPerson
Copy link
Contributor Author

@smeech Thank you very much for your guidance. I have learned a better way of configuration from this pr

@smeech smeech merged commit 3d16206 into espanso:main Jun 13, 2024
1 check passed
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.

2 participants